diff --git a/actions/setup/js/trace_graders.cjs b/actions/setup/js/trace_graders.cjs index dcc0f56ed29..b039a521f30 100644 --- a/actions/setup/js/trace_graders.cjs +++ b/actions/setup/js/trace_graders.cjs @@ -880,13 +880,18 @@ function executeCustomGraderInSubprocess(id, script, trace, meta) { timeoutMs: SCRIPT_TIMEOUT_MS, }; const safeEnv = {}; - for (const key of ["PATH", "HOME", "TMPDIR", "TEMP", "TMP", "SystemRoot", "ComSpec"]) { + for (const key of ["SystemRoot"]) { if (process.env[key]) { safeEnv[key] = process.env[key]; } } const timeoutMs = SCRIPT_TIMEOUT_MS + SCRIPT_WORKER_OVERHEAD_MS; - const proc = cp.spawnSync(process.execPath, [SCRIPT_WORKER_PATH], { + const permissionFlag = process.allowedNodeEnvironmentFlags.has("--permission") ? "--permission" : process.allowedNodeEnvironmentFlags.has("--experimental-permission") ? "--experimental-permission" : null; + if (!permissionFlag) { + throw new Error("Node.js permission support is required to run custom graders; install Node.js 20 or newer"); + } + const workerPath = fs.realpathSync(SCRIPT_WORKER_PATH); + const proc = cp.spawnSync(process.execPath, [permissionFlag, `--allow-fs-read=${workerPath}`, workerPath], { input: JSON.stringify(payload), encoding: "utf-8", timeout: timeoutMs, diff --git a/actions/setup/js/trace_graders.test.cjs b/actions/setup/js/trace_graders.test.cjs index 2966cb66e57..3f1fe565c2e 100644 --- a/actions/setup/js/trace_graders.test.cjs +++ b/actions/setup/js/trace_graders.test.cjs @@ -549,20 +549,77 @@ printf '%s\\n' '[{"id":"goal-attained","value":0.75},{"id":"evidence-available", expect(result.value).toBeNull(); expect(result.error).toContain("non-finite"); }); + + it("rejects non-finite values in object results", () => { + const result = runCustomGrader("test", "return { value: NaN }", makeTrace(), { name: "test", unit: "", direction: "", source: "inline" }); + expect(result.value).toBeNull(); + expect(result.status).toBe("error"); + expect(result.error).toContain("non-finite"); + }); }); // --- runCustomGrader node:vm sandbox --- describe("runCustomGrader sandbox", () => { const meta = { name: "test", unit: "", direction: "", source: "inline" }; - it("cannot access require", () => { - const result = runCustomGrader("test", "return typeof require", makeTrace(), meta); - expect(result.value).toBeNull(); // "undefined" is not a number + it("selects the supported permission flag", () => { + const cp = require("child_process"); + const originalFlags = process.allowedNodeEnvironmentFlags; + for (const flag of ["--permission", "--experimental-permission"]) { + process.allowedNodeEnvironmentFlags = new Set([flag]); + const spawn = vi.spyOn(cp, "spawnSync").mockReturnValue({ status: 0, stdout: '{"ok":true,"value":1}' }); + try { + expect(runCustomGrader("test", "return 1", makeTrace(), meta).value).toBe(1); + expect(spawn.mock.calls[0][1][0]).toBe(flag); + expect(spawn.mock.calls[0][1][1]).toBe(`--allow-fs-read=${spawn.mock.calls[0][1][2]}`); + } finally { + process.allowedNodeEnvironmentFlags = originalFlags; + spawn.mockRestore(); + } + } }); - it("cannot access process", () => { - const result = runCustomGrader("test", "return typeof process", makeTrace(), meta); - expect(result.value).toBeNull(); // "undefined" is not a number + it("fails closed when the permission model is unavailable", () => { + const originalFlags = process.allowedNodeEnvironmentFlags; + process.allowedNodeEnvironmentFlags = new Set(); + try { + const result = runCustomGrader("test", "return 1", makeTrace(), meta); + expect(result.status).toBe("error"); + expect(result.value).toBeNull(); + expect(result.error).toContain("install Node.js 20 or newer"); + } finally { + process.allowedNodeEnvironmentFlags = originalFlags; + } + }); + + it("cannot access process or require", () => { + const result = runCustomGrader("test", "return typeof process === 'undefined' && typeof require === 'undefined' ? 1 : 0", makeTrace(), meta); + expect(result.value).toBe(1); + expect(result.status).toBe("pass"); + }); + + it("does not expose the bootstrap global's host constructor", () => { + const result = runCustomGrader("test", 'try { return typeof root.constructor.constructor("return process")() === "object" ? 1 : 0; } catch { return 0; }', makeTrace(), meta); + expect(result.value).toBe(0); + expect(result.status).toBe("pass"); + }); + + it("does not let a host-object constructor escape execute a command", () => { + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "grader-escape-")); + const markerPath = path.join(tempDir, "executed"); + const script = `try { + const escapedProcess = trace.constructor.constructor("return process")(); + escapedProcess.getBuiltinModule("node:child_process").execSync("touch ${markerPath}"); + } catch {} + return 1;`; + try { + const result = runCustomGrader("test", script, makeTrace(), meta); + expect(result.value).toBe(1); + expect(result.status).toBe("pass"); + expect(fs.existsSync(markerPath)).toBe(false); + } finally { + fs.rmSync(tempDir, { recursive: true, force: true }); + } }); it("cannot access fetch", () => { diff --git a/actions/setup/js/trace_graders_worker.cjs b/actions/setup/js/trace_graders_worker.cjs index 53004ef48e8..17a255bdf11 100644 --- a/actions/setup/js/trace_graders_worker.cjs +++ b/actions/setup/js/trace_graders_worker.cjs @@ -2,36 +2,6 @@ const vm = require("vm"); -/** - * @param {any} value - * @param {string} label - * @returns {any} - */ -function tryStructuredCloneOrUndefined(value, label) { - try { - return structuredClone(value); - } catch { - process.stderr.write(`grader worker: failed to structuredClone ${label}; value will default to {}\n`); - return undefined; - } -} - -/** - * @param {any} obj - * @returns {any} - */ -function deepFreeze(obj) { - if (obj === null || typeof obj !== "object") return obj; - Object.freeze(obj); - for (const key of Object.getOwnPropertyNames(obj)) { - const value = obj[key]; - if (value !== null && typeof value === "object" && !Object.isFrozen(value)) { - deepFreeze(value); - } - } - return obj; -} - function readStdin() { return new Promise(resolve => { let data = ""; @@ -55,35 +25,53 @@ async function main() { } try { - const trace = deepFreeze(tryStructuredCloneOrUndefined(payload.trace, "trace") ?? {}); - const config = deepFreeze(tryStructuredCloneOrUndefined(payload.config, "config") ?? {}); - const run = deepFreeze({ graderCount: Number(payload.graderCount) || 0 }); - const workflow = deepFreeze({}); const script = String(payload.script || ""); - const sandbox = { - trace, - run, - workflow, - config, + const sandbox = Object.assign(Object.create(null), { + __payload: JSON.stringify({ + trace: payload.trace || {}, + run: { graderCount: Number(payload.graderCount) || 0 }, + workflow: {}, + config: payload.config || {}, + }), Date: undefined, fetch: undefined, require: undefined, process: undefined, global: undefined, - globalThis: undefined, Function: undefined, eval: undefined, undefined, NaN, Infinity, - }; + }); const context = vm.createContext(sandbox, { codeGeneration: { strings: false, wasm: false } }); const timeoutMs = Number(payload.timeoutMs) || 5000; - const runtimeBindings = vm.runInContext( + vm.runInContext( ` "use strict"; + (() => { + const root = globalThis; + const deepFreeze = value => { + if (value === null || typeof value !== "object") return value; + Object.freeze(value); + for (const key of Object.getOwnPropertyNames(value)) { + if (value[key] !== null && typeof value[key] === "object" && !Object.isFrozen(value[key])) { + deepFreeze(value[key]); + } + } + return value; + }; + const data = JSON.parse(__payload); + for (const [key, value] of Object.entries(data)) { + Object.defineProperty(root, key, { + value: deepFreeze(value), + writable: false, + enumerable: true, + configurable: false + }); + } (() => { const m = {}; const descriptors = Object.getOwnPropertyDescriptors(Math); @@ -97,44 +85,58 @@ async function main() { configurable: false }); const safeMath = Object.freeze(m); - const helpers = Object.freeze({ + Object.defineProperty(root, "__math", { + value: safeMath, + writable: false, + enumerable: false, + configurable: false + }); + Object.defineProperty(root, "helpers", { + value: Object.freeze({ clamp: (v, lo, hi) => safeMath.max(lo, safeMath.min(hi, v)), ratio: (num, den) => (den === 0 ? 0 : num / den), sum: arr => arr.reduce((a, b) => a + b, 0) + }), + writable: false, + enumerable: true, + configurable: false }); - return { safeMath, helpers }; + })(); + delete root.__payload; + Object.defineProperty(root, "globalThis", { + value: undefined, + writable: false, + enumerable: false, + configurable: false + }); })(); `, context, { timeout: 1000, filename: "grader:bootstrap" } ); - Object.defineProperty(context, "helpers", { - value: runtimeBindings.helpers, - writable: false, - enumerable: true, - configurable: false, - }); - Object.defineProperty(context, "__math", { - value: runtimeBindings.safeMath, - writable: false, - enumerable: false, - configurable: false, - }); const graderFn = vm.compileFunction(`"use strict";\n${script}`, ["trace", "run", "workflow", "config", "helpers", "Math"], { parsingContext: context, filename: `grader:${String(payload.id || "unknown")}`, }); context.__grader = graderFn; - const value = vm.runInContext("__grader(trace, run, workflow, config, helpers, __math)", context, { - timeout: timeoutMs, - filename: `grader:${String(payload.id || "unknown")}:invoke`, - }); - if (typeof value === "number" && !Number.isFinite(value)) { - throw new Error("custom grader returned non-finite numeric value"); - } - - process.stdout.write(JSON.stringify({ ok: true, value })); + const result = vm.runInContext( + `(() => { + try { + const value = __grader(trace, run, workflow, config, helpers, __math); + const metricValue = value !== null && typeof value === "object" && Object.hasOwn(value, "value") ? value.value : value; + if (typeof metricValue === "number" && !Number.isFinite(metricValue)) { + throw new Error("custom grader returned non-finite numeric value"); + } + return JSON.stringify({ ok: true, value }); + } catch (err) { + return JSON.stringify({ ok: false, error: err instanceof Error ? err.message : String(err) }); + } + })()`, + context, + { timeout: timeoutMs, filename: `grader:${String(payload.id || "unknown")}:invoke` } + ); + process.stdout.write(result); } catch (err) { const message = err instanceof Error ? err.message : String(err); process.stdout.write(JSON.stringify({ ok: false, error: message })); diff --git a/docs/src/content/docs/experimental/drive-memory.md b/docs/src/content/docs/experimental/drive-memory.md index 8de2cd071d8..4a8394e7ee2 100644 --- a/docs/src/content/docs/experimental/drive-memory.md +++ b/docs/src/content/docs/experimental/drive-memory.md @@ -24,7 +24,7 @@ Drive names are repository-wide and branch-aware according to the preview servic Drive-memory supports the same `validation.json-schemas` declarations as repo-memory and cache-memory. Each declaration has an inline `schema` and may specify a relative file path or glob (with `*` within a path segment and `**` across segments). If `file` is omitted, the schema applies to every eligible `.json` and `.jsonl` file in the memory directory, excluding `.git`; omit `format` too so the format is inferred for each file. Otherwise, each path must exist, each glob must match at least one file, and every matched file must pass validation before the drive accepts or persists the candidate. The format is inferred from each matched file's `.json` or `.jsonl` extension (case-insensitive). Optional `format: json` or `format: jsonl` overrides inference and is required for other extensions. -JSON requires one non-empty document. JSONL validates each physical record independently, accepts LF/CRLF and an optional final newline, permits an existing empty file, and rejects blank or malformed records with a line number. Schema validation runs after filtering and configured normalization, before an optional `validation.script`, and gates drive persistence. The supported vocabulary is `type`, primitive `enum`, `required`, nested `properties`, `additionalProperties: false`, `items`, and standalone `oneOf`/`anyOf` (maximum depth 32); this is not full JSON Schema. Numeric enum integers must be exactly representable by JavaScript, and values checked against numeric enums must not lose precision during JSON parsing. Unsupported keywords are rejected at compile time. A script remains useful for cross-file or domain-specific rules. See [Cache Memory](../reference/cache-memory/) for a complete example and details. +JSON requires one non-empty document. JSONL validates each physical record independently, accepts LF/CRLF and an optional final newline, permits an existing empty file, and rejects blank or malformed records with a line number. Schema validation runs after filtering and configured normalization, before an optional `validation.script`, and gates drive persistence. The supported vocabulary is `type`, primitive `enum`, `required`, nested `properties`, `additionalProperties: false`, `items`, and standalone `oneOf`/`anyOf` (maximum depth 32); this is not full JSON Schema. Numeric enum integers must be exactly representable by JavaScript, and values checked against numeric enums must not lose precision during JSON parsing. Unsupported keywords are rejected at compile time. A script remains useful for cross-file or domain-specific rules. See [Cache Memory](/gh-aw/reference/cache-memory/) for a complete example and details. ## Drive size diff --git a/docs/src/content/docs/reference/glossary.md b/docs/src/content/docs/reference/glossary.md index cb9cf8ffe35..bfa5d851a3c 100644 --- a/docs/src/content/docs/reference/glossary.md +++ b/docs/src/content/docs/reference/glossary.md @@ -1723,7 +1723,7 @@ An optional field under top-level `metadata:` frontmatter holding an absolute HT ### Graders (`graders:`) -Deterministic, non-LLM checks that compute metrics from a workflow run's post-agent execution trace. Configured under the top-level `graders:` frontmatter field; an empty map (`graders: {}`) enables all built-in graders with default settings, and omitting the field disables grading entirely. Built-in graders cover tool success rate, retries, loop detection, trajectory efficiency, execution duration, and similar metrics. Custom inline graders run a trusted, sandboxed JavaScript expression against the preprocessed `trace` object. Graders are an experimental feature. See [Graders Reference](/gh-aw/experimental/trace-graders/). +Deterministic, non-LLM checks that compute metrics from a workflow run's post-agent execution trace. Configured under the top-level `graders:` frontmatter field; an empty map (`graders: {}`) enables all built-in graders with default settings, and omitting the field disables grading entirely. Built-in graders cover tool success rate, retries, loop detection, trajectory efficiency, execution duration, and similar metrics. Custom inline graders run in a separate restricted Node.js process against the preprocessed `trace` object, without access to `process` or `require`. Graders are an experimental feature. See [Graders Reference](/gh-aw/experimental/trace-graders/). ### Operational Value Grader (`graders.operational-value`) diff --git a/docs/src/content/docs/setup/cli.md b/docs/src/content/docs/setup/cli.md index c76e3684d9b..fbdd9a946dc 100644 --- a/docs/src/content/docs/setup/cli.md +++ b/docs/src/content/docs/setup/cli.md @@ -670,6 +670,11 @@ executes either Bash embedded in `graders.operational-value.script` or a Bash file referenced by `graders.operational-value.run`; operational-value graders require standard input because historical replay is not supported. +Built-in and custom JavaScript graders require Node.js 20 or newer with the +permission model available. The CLI selects `--permission` or +`--experimental-permission` according to runtime support and never runs a +JavaScript grader without permissions enabled. + ```bash wrap gh aw graders run weekly-research loops 123456789 cat payload.json | gh aw graders run weekly-research loops diff --git a/docs/src/content/docs/specs/graders-specification.md b/docs/src/content/docs/specs/graders-specification.md index 262ac26cdfa..1ecbb77b080 100644 --- a/docs/src/content/docs/specs/graders-specification.md +++ b/docs/src/content/docs/specs/graders-specification.md @@ -193,21 +193,6 @@ A custom grader MUST define `script`. - `script` MUST be non-empty. - `script` MUST NOT exceed 4096 characters. -### 6.3 Forbidden Patterns - -Inline scripts MUST be rejected if they contain any forbidden pattern, including: - -- `require(` -- `import(` -- `import ` -- `fetch(` -- `eval(` -- `process.exit` -- `child_process` -- `execSync` -- `spawnSync` -- `Function(` - --- ## 7. Operational Value Grader @@ -288,7 +273,9 @@ semantic task correctness. The normative readiness, decision, and JSON contracts ## 10. Security and Isolation - Grading MUST operate on local run artifacts and MUST NOT require outbound network access for built-ins. -- Custom inline graders MUST execute in a restricted context with blocked dangerous primitives. +- Custom inline graders MUST execute in a separate Node.js process with the permission model enabled and a minimal environment. +- Grader inputs MUST be reconstructed inside the JavaScript context; host-created objects and functions MUST NOT be exposed to the grader. +- `process`, `require`, and other host capabilities MUST NOT be available in the grader context. Source-pattern blocklists MUST NOT be relied on as a security boundary. - Operational-value graders MAY access declared repository evidence using `GH_TOKEN`; implementations MUST NOT add agent-job permission scopes on behalf of the evaluator, and evaluators MUST NOT receive workflow secrets. - Implementations SHOULD enforce bounded execution time for inline scripts. - Implementations SHOULD redact grader outputs when custom scripts are enabled to reduce secret leakage risk. @@ -364,5 +351,5 @@ semantic task correctness. The normative readiness, decision, and JSON contracts - Initial draft for gh-aw graders. - Defines `graders` configuration semantics and built-in grader set. -- Defines custom inline grader constraints and forbidden patterns. +- Defines custom inline grader constraints. - Defines grader artifact output contract and experiment metric references. diff --git a/pkg/cli/graders_run.cjs b/pkg/cli/graders_run.cjs index 344346b6869..31ea22f021e 100644 --- a/pkg/cli/graders_run.cjs +++ b/pkg/cli/graders_run.cjs @@ -54,47 +54,74 @@ function deepFreeze(value) { } function runInline(grader, trace) { - const sandbox = { - trace: deepFreeze(structuredClone(trace)), - run: deepFreeze({ graderCount: 1 }), - workflow: deepFreeze({}), - config: deepFreeze(structuredClone(grader.config || {})), + const sandbox = Object.assign(Object.create(null), { + __payload: JSON.stringify({ + trace, + run: { graderCount: 1 }, + workflow: {}, + config: grader.config || {}, + }), Date: undefined, fetch: undefined, require: undefined, process: undefined, global: undefined, - globalThis: undefined, Function: undefined, eval: undefined, - }; + }); const context = vm.createContext(sandbox, { codeGeneration: { strings: false, wasm: false } }); - const safeMath = vm.runInContext( + vm.runInContext( `(() => { - const value = {}; - Object.defineProperties(value, Object.getOwnPropertyDescriptors(Math)); - Object.defineProperty(value, "random", { value: undefined }); - return Object.freeze(value); + const root = globalThis; + const deepFreeze = value => { + if (value === null || typeof value !== "object") return value; + Object.freeze(value); + for (const key of Object.getOwnPropertyNames(value)) { + if (value[key] !== null && typeof value[key] === "object" && !Object.isFrozen(value[key])) deepFreeze(value[key]); + } + return value; + }; + const input = JSON.parse(__payload); + for (const [key, value] of Object.entries(input)) { + Object.defineProperty(root, key, { value: deepFreeze(value), writable: false, enumerable: true, configurable: false }); + } + const math = {}; + Object.defineProperties(math, Object.getOwnPropertyDescriptors(Math)); + Object.defineProperty(math, "random", { value: undefined }); + const safeMath = Object.freeze(math); + Object.defineProperty(root, "__math", { value: safeMath, writable: false, configurable: false }); + Object.defineProperty(root, "helpers", { + value: Object.freeze({ + clamp: (value, low, high) => safeMath.max(low, safeMath.min(high, value)), + ratio: (numerator, denominator) => (denominator === 0 ? 0 : numerator / denominator), + sum: values => values.reduce((sum, value) => sum + value, 0), + }), + writable: false, + enumerable: true, + configurable: false, + }); + delete root.__payload; + Object.defineProperty(root, "globalThis", { value: undefined, writable: false, configurable: false }); })()`, context, { timeout: 1000 } ); - const helpers = deepFreeze({ - clamp: (value, low, high) => Math.max(low, Math.min(high, value)), - ratio: (numerator, denominator) => (denominator === 0 ? 0 : numerator / denominator), - sum: values => values.reduce((sum, value) => sum + value, 0), - }); const fn = vm.compileFunction(`"use strict";\n${grader.script}`, ["trace", "run", "workflow", "config", "helpers", "Math"], { parsingContext: context, filename: `grader:${grader.id}`, }); context.__grader = fn; - context.__helpers = helpers; - context.__math = safeMath; - return vm.runInContext("__grader(trace, run, workflow, config, __helpers, __math)", context, { - timeout: 5000, - filename: `grader:${grader.id}:invoke`, - }); + const result = vm.runInContext( + `(() => { + const raw = __grader(trace, run, workflow, config, helpers, __math); + const value = raw !== null && typeof raw === "object" && Object.hasOwn(raw, "value") ? raw.value : raw; + return JSON.stringify(typeof value === "number" && !Number.isFinite(value) ? { nonFinite: String(value) } : { raw }); + })()`, + context, + { timeout: 5000, filename: `grader:${grader.id}:invoke` } + ); + const parsed = JSON.parse(result); + return Object.hasOwn(parsed, "nonFinite") ? Number(parsed.nonFinite) : parsed.raw; } function normalize(grader, raw) { diff --git a/pkg/cli/graders_run.go b/pkg/cli/graders_run.go index 40385b9f37d..33a168a017d 100644 --- a/pkg/cli/graders_run.go +++ b/pkg/cli/graders_run.go @@ -203,6 +203,10 @@ func runJavaScriptGrader(ctx context.Context, grader graderRunDefinition, payloa if err != nil { return errors.New("node is required to run graders") } + permissionFlag, err := graderNodePermissionFlag(ctx, nodePath) + if err != nil { + return err + } script, err := os.CreateTemp("", "gh-aw-grader-run-*.cjs") if err != nil { return fmt.Errorf("failed to stage grader runtime: %w", err) @@ -210,16 +214,18 @@ func runJavaScriptGrader(ctx context.Context, grader graderRunDefinition, payloa scriptPath := script.Name() defer os.Remove(scriptPath) if err := script.Chmod(constants.FilePermSensitive); err != nil { - _ = script.Close() - return fmt.Errorf("failed to secure grader runtime: %w", err) + return fmt.Errorf("failed to secure grader runtime: %w", errors.Join(err, script.Close())) } if _, err := script.Write(gradersRunScript); err != nil { - _ = script.Close() - return fmt.Errorf("failed to stage grader runtime: %w", err) + return fmt.Errorf("failed to stage grader runtime: %w", errors.Join(err, script.Close())) } if err := script.Close(); err != nil { return fmt.Errorf("failed to stage grader runtime: %w", err) } + scriptPath, err = filepath.EvalSymlinks(scriptPath) + if err != nil { + return fmt.Errorf("failed to resolve grader runtime path: %w", err) + } input, err := json.Marshal(struct { Grader graderRunDefinition `json:"grader"` Payload json.RawMessage `json:"payload"` @@ -229,7 +235,8 @@ func runJavaScriptGrader(ctx context.Context, grader graderRunDefinition, payloa } commandCtx, cancel := context.WithTimeout(ctx, graderJSTimeout) defer cancel() - cmd := exec.CommandContext(commandCtx, nodePath, scriptPath) + cmd := exec.CommandContext(commandCtx, nodePath, permissionFlag, "--allow-fs-read="+scriptPath, scriptPath) + cmd.Env = []string{} cmd.Stdin = bytes.NewReader(input) cmd.Stdout = output stderr := &boundedCommandBuffer{limit: maxOperationalValueOutputBytes} @@ -246,6 +253,28 @@ func runJavaScriptGrader(ctx context.Context, grader graderRunDefinition, payloa return nil } +func graderNodePermissionFlag(ctx context.Context, nodePath string) (string, error) { + probeCtx, cancel := context.WithTimeout(ctx, graderJSTimeout) + defer cancel() + cmd := exec.CommandContext(probeCtx, nodePath, "--print", + `process.allowedNodeEnvironmentFlags.has("--permission") ? "--permission" : process.allowedNodeEnvironmentFlags.has("--experimental-permission") ? "--experimental-permission" : ""`) + cmd.Env = []string{} + stdout := &boundedCommandBuffer{limit: 1024} + stderr := &boundedCommandBuffer{limit: 1024} + cmd.Stdout = stdout + cmd.Stderr = stderr + if err := cmd.Run(); err != nil { + return "", fmt.Errorf("failed to detect Node.js permission support: %w: %s", err, bytes.TrimSpace(stderr.Bytes())) + } + flag := strings.TrimSpace(stdout.String()) + switch flag { + case "--permission", "--experimental-permission": + return flag, nil + default: + return "", errors.New("node permission support is required to run graders; install Node.js 20 or newer") + } +} + func runOperationalValuePayload(ctx context.Context, workflowArg string, grader graderRunDefinition, payload json.RawMessage, output io.Writer, evaluatorHost string) error { evaluatorContent, err := loadOperationalValueEvaluatorContent(workflowArg, grader) if err != nil { @@ -362,7 +391,9 @@ func (buffer *boundedCommandBuffer) Write(data []byte) (int, error) { remaining := buffer.limit - buffer.Len() if remaining > 0 { remaining = min(len(data), remaining) - _, _ = buffer.Buffer.Write(data[:remaining]) + if _, err := buffer.Buffer.Write(data[:remaining]); err != nil { + return 0, err + } } if written > remaining { buffer.exceeded = true diff --git a/pkg/cli/graders_run_test.go b/pkg/cli/graders_run_test.go index f3285356d59..91c049131f6 100644 --- a/pkg/cli/graders_run_test.go +++ b/pkg/cli/graders_run_test.go @@ -3,8 +3,11 @@ package cli import ( "bytes" "context" + "encoding/json" + "fmt" "os" "path/filepath" + "runtime" "testing" "github.com/stretchr/testify/assert" @@ -90,6 +93,83 @@ graders: }`, output.String()) } +func TestRunInlineScriptGraderCannotAccessProcessOrRequire(t *testing.T) { + workflowID := writeGraderRunWorkflow(t, `--- +graders: + custom-score: + script: | + return typeof process === "undefined" && typeof require === "undefined" ? 1 : 0 +--- +`) + var output bytes.Buffer + err := runGrader(context.Background(), graderRunConfig{ + Workflow: workflowID, + GraderID: "custom-score", + Input: bytes.NewBufferString(`{}`), + Output: &output, + }) + require.NoError(t, err) + var result struct { + Value float64 `json:"value"` + } + require.NoError(t, json.Unmarshal(output.Bytes(), &result)) + require.InDelta(t, float64(1), result.Value, 0) +} + +func TestRunInlineScriptGraderNonFiniteValues(t *testing.T) { + for _, script := range []string{"return NaN", "return Infinity", "return -Infinity", "return { value: NaN }"} { + t.Run(script, func(t *testing.T) { + workflowID := writeGraderRunWorkflow(t, fmt.Sprintf("---\ngraders:\n custom-score:\n script: |\n %s\n---\n", script)) + var output bytes.Buffer + require.NoError(t, runGrader(context.Background(), graderRunConfig{ + Workflow: workflowID, + GraderID: "custom-score", + Input: bytes.NewBufferString(`{}`), + Output: &output, + })) + var result struct { + Value *float64 `json:"value"` + Status string `json:"status"` + Error string `json:"error"` + } + require.NoError(t, json.Unmarshal(output.Bytes(), &result)) + assert.Nil(t, result.Value) + assert.Equal(t, "error", result.Status) + assert.Contains(t, result.Error, "non-finite value") + }) + } +} + +func TestRunInlineScriptGraderCannotEscapeAndExecuteCommand(t *testing.T) { + markerPath := filepath.Join(t.TempDir(), "executed") + workflowID := writeGraderRunWorkflow(t, fmt.Sprintf(`--- +graders: + custom-score: + script: | + try { + const escapedProcess = trace.constructor.constructor("return process")() + escapedProcess.getBuiltinModule("node:child_process").execSync("touch %s") + } catch {} + return 1 +--- +`, markerPath)) + var output bytes.Buffer + err := runGrader(context.Background(), graderRunConfig{ + Workflow: workflowID, + GraderID: "custom-score", + Input: bytes.NewBufferString(`{}`), + Output: &output, + }) + require.NoError(t, err) + var result struct { + Value float64 `json:"value"` + } + require.NoError(t, json.Unmarshal(output.Bytes(), &result)) + require.InDelta(t, float64(1), result.Value, 0) + _, err = os.Stat(markerPath) + require.ErrorIs(t, err, os.ErrNotExist) +} + func TestRunInlineOperationalValueGraderFromStdin(t *testing.T) { workflowID := writeGraderRunWorkflow(t, `--- graders: @@ -170,6 +250,41 @@ func TestReadGraderPayloadValidation(t *testing.T) { require.ErrorContains(t, err, "positive integer") } +func TestGraderNodePermissionFlag(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("uses a POSIX shell fixture") + } + for _, test := range []struct { + name string + output string + flag string + wantErr string + }{ + {name: "stable", output: "--permission", flag: "--permission"}, + {name: "experimental Node 20", output: "--experimental-permission", flag: "--experimental-permission"}, + {name: "unsupported", wantErr: "install Node.js 20 or newer"}, + {name: "unexpected output", output: "--allow-fs-read=*", wantErr: "permission support is required"}, + } { + t.Run(test.name, func(t *testing.T) { + nodePath := filepath.Join(t.TempDir(), "node") + require.NoError(t, os.WriteFile(nodePath, []byte(fmt.Sprintf("#!/bin/sh\nprintf '%%s\\n' '%s'\n", test.output)), 0o700)) + flag, err := graderNodePermissionFlag(context.Background(), nodePath) + if test.wantErr != "" { + require.ErrorContains(t, err, test.wantErr) + assert.Empty(t, flag) + } else { + require.NoError(t, err) + assert.Equal(t, test.flag, flag) + } + }) + } +} + +func TestGraderNodePermissionFlagProbeFailure(t *testing.T) { + _, err := graderNodePermissionFlag(context.Background(), filepath.Join(t.TempDir(), "missing-node")) + require.ErrorContains(t, err, "failed to detect Node.js permission support") +} + func TestValidateOperationalValueEvaluatorSource(t *testing.T) { valid := []byte("#!/usr/bin/env bash\nprintf '%s\\n' '[]'\n") actual, err := validateOperationalValueEvaluatorSource(valid) diff --git a/pkg/workflow/graders_config.go b/pkg/workflow/graders_config.go index 68ae14dd9aa..720a728bd0e 100644 --- a/pkg/workflow/graders_config.go +++ b/pkg/workflow/graders_config.go @@ -451,12 +451,6 @@ func parseGraderEntryFields(def *GraderDefinition, entry map[string]any, id stri if scriptCharCount > 4096 { return fmt.Errorf("graders.%s.script exceeds maximum length of 4096 characters (%d)", id, scriptCharCount) } - forbiddenPatterns := []string{"require(", "import(", "import ", "fetch(", "eval(", "process.exit", "child_process", "execSync", "spawnSync", "Function("} - for _, p := range forbiddenPatterns { - if strings.Contains(s, p) { - return fmt.Errorf("graders.%s.script contains forbidden pattern %q — inline grader scripts must be pure functions without side effects", id, p) - } - } def.Script = s } diff --git a/pkg/workflow/graders_config_test.go b/pkg/workflow/graders_config_test.go index 87de6a987b4..c13fc2d913c 100644 --- a/pkg/workflow/graders_config_test.go +++ b/pkg/workflow/graders_config_test.go @@ -404,27 +404,24 @@ func TestParseGradersFromFrontmatter_InvalidType(t *testing.T) { } } -// TestParseGradersFromFrontmatter_ForbiddenScript verifies forbidden patterns in scripts. -func TestParseGradersFromFrontmatter_ForbiddenScript(t *testing.T) { +// TestParseGradersFromFrontmatter_CustomGraderDoesNotUseSourceBlocklist verifies scripts are not filtered by source substrings. +func TestParseGradersFromFrontmatter_CustomGraderDoesNotUseSourceBlocklist(t *testing.T) { var c Compiler - forbidden := []string{ - "require('fs')", - "import('os')", - "fetch('http://evil.com')", - "eval('bad')", - "process.exit(1)", - } - for _, script := range forbidden { + scripts := []string{ + "return typeof require", + "return typeof process", + "return 'fetch('", + "return 'child_process execSync spawnSync'", + "return 'import('", + } + for _, script := range scripts { _, err := c.parseGradersFromFrontmatter(map[string]any{ "graders": map[string]any{ - "bad-grader": map[string]any{"script": script}, + "custom-grader": map[string]any{"script": script}, }, }) - if err == nil { - t.Fatalf("expected error for forbidden script: %s", script) - } - if !strings.Contains(err.Error(), "forbidden pattern") { - t.Fatalf("expected forbidden pattern error, got: %v", err) + if err != nil { + t.Errorf("unexpected error for %q: %v", script, err) } } }