From 17874c29b3cd308a9c4c896ca2b88f7f0f528009 Mon Sep 17 00:00:00 2001 From: Matthias Linhuber Date: Sat, 10 Oct 2026 23:03:13 +0200 Subject: [PATCH 1/2] fix(security): remove silent gradlew RCE, stop leaking token and JWT, validate ARTEMIS_URL Addresses four findings from the security audit of the env-var-passthrough work: - Gradle prewarm no longer auto-executes the cloned repo's ./gradlew wrapper. The student must explicitly confirm before any repository-controlled Gradle build runs, and confirmation defaults to "no". This removes a silent remote-code-execution path from tampered exercise templates while keeping prewarm available as an opt-in. - The VCS/Git token is no longer embedded in the clone URL or passed on the git command line. It is injected into the git process via GIT_CONFIG_* environment variables during clone and persisted only into .git/config for later in-IDE fetch/push. The unused addVcsTokenToUrl helper is removed. Residual: the short-lived token is still written to .git/config so push/fetch keep working. - STOMP/WebSocket frame logging is disabled so the Artemis JWT in the CONNECT frame is never written to the debug console. - The ARTEMIS_URL environment override is validated against the configured Artemis origin (scheme + host + port) before being used as the API base, preventing the Bearer token from being sent to an untrusted host. Adds unit tests for the clone auth handling, the gradle confirmation gate and the ARTEMIS_URL validation. Co-Authored-By: Claude Opus 4.8 --- .gitignore | 3 +- CHANGELOG.md | 14 +++++ shared/models/participation.model.ts | 11 ---- src/participation/cloning.service.ts | 65 +++++++++++++++------ src/participation/gradle.service.ts | 85 +++++++++++++++++++++------- src/shared/artemis-url.ts | 45 +++++++++++++++ src/shared/settings.ts | 17 +++++- src/shared/websocket.ts | 7 ++- src/test/suite/artemis-url.test.ts | 44 ++++++++++++++ src/test/suite/cloning.test.ts | 61 +++++++++++++++----- src/test/suite/gradle.test.ts | 81 ++++++++++++++++++++++++++ 11 files changed, 365 insertions(+), 68 deletions(-) create mode 100644 src/shared/artemis-url.ts create mode 100644 src/test/suite/artemis-url.test.ts create mode 100644 src/test/suite/gradle.test.ts diff --git a/.gitignore b/.gitignore index 15bedd29..f653d572 100644 --- a/.gitignore +++ b/.gitignore @@ -11,4 +11,5 @@ tsconfig.lsif.json *.vsix .env .DS_Store -secret.txt \ No newline at end of file +secret.txt +.vscode-test-web diff --git a/CHANGELOG.md b/CHANGELOG.md index c5c4922b..7cd078f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,3 +7,17 @@ Check [Keep a Changelog](http://keepachangelog.com/) for recommendations on how ## [Unreleased] - Initial release + +### Security + +- Gradle prewarm no longer runs the cloned repository's `./gradlew` wrapper automatically. The + student must explicitly confirm before any repository-controlled Gradle build is executed, + removing a silent remote-code-execution path from tampered exercise templates. +- The VCS/Git access token is no longer embedded in the clone URL or passed on the git command + line. It is injected through the git process environment (`GIT_CONFIG_*`) during clone and + persisted only into `.git/config` for later in-IDE fetch/push. +- STOMP/WebSocket frame logging is disabled so the Artemis JWT in the CONNECT frame is never + written to the debug console. +- The `ARTEMIS_URL` environment override is validated against the configured Artemis origin + (scheme + host + port) before it is used as the API base, preventing the Bearer token from + being sent to an untrusted host. diff --git a/shared/models/participation.model.ts b/shared/models/participation.model.ts index fe4275ad..405a2dd9 100644 --- a/shared/models/participation.model.ts +++ b/shared/models/participation.model.ts @@ -60,14 +60,3 @@ export function getProjectKeyFromRepoUrl(repoUrl: string): string { const projectKey = parts[4]; return projectKey; } - -export function addVcsTokenToUrl(url: string, username: string, vsctoken: string): string { - const credentials = `://${username}:${vsctoken}@`; - if (!url.includes("@")) { - // the url has the format https://vcs-server.com - return url.replace("://", credentials); - } else { - // the url has the format https://username@vcs-server.com -> replace ://username@ - return url.replace(/:\/\/.*@/, credentials); - } -} diff --git a/src/participation/cloning.service.ts b/src/participation/cloning.service.ts index 955433a7..10d1a76a 100644 --- a/src/participation/cloning.service.ts +++ b/src/participation/cloning.service.ts @@ -3,13 +3,12 @@ import { settings } from "../shared/settings"; import { NotAuthenticatedError } from "../authentication/not_authenticated.error"; import { AUTH_ID } from "../authentication/authentication_provider"; import { getState } from "../shared/state"; -import simpleGit from "simple-git"; +import simpleGit, { GitConfigScope } from "simple-git"; import { tmpdir } from "os"; import * as path from "path"; import * as fs from "fs/promises"; import { retrieveVcsAccessToken } from "../artemis/authentication.client"; import { getWorkspaceFolder, theiaEnv } from "../theia/theia"; -import { addVcsTokenToUrl } from "@shared/models/participation.model"; import { ProgrammingExercise, ProgrammingLanguage } from "@shared/models/exercise.model"; import { warmupGradleDaemon } from "./gradle.service"; @@ -19,8 +18,10 @@ type CloneOptions = { mode?: CloneMode; preservedPaths?: string[]; // Full HTTP header line (e.g. "Authorization: Bearer ") to authenticate the clone. - // When set, it is passed as `git -c http.extraHeader=...` and persisted into the cloned - // repo's .git/config so subsequent in-IDE fetch/push stay authenticated. + // When set, it is injected into the git process via the GIT_CONFIG_* environment variables + // (never on the command line, so the token does not appear in process arguments) and then + // persisted into the cloned repo's .git/config so subsequent in-IDE fetch/push stay + // authenticated. httpExtraHeader?: string; }; @@ -79,15 +80,21 @@ export async function cloneUserRepo(repoUrl: string, username: string) { session.accessToken, getState().displayedExercise?.studentParticipations![0].id!, ); - // Clone the repository - const cloneUrlWithToken = new URL(addVcsTokenToUrl(repoUrl, username, vcsToken)); - const clonePath = await cloneByGivenURL(cloneUrlWithToken, destinationPath, cloneOptions); + // Authenticate the clone with an HTTP Basic header instead of embedding `user:token@host` + // in the URL. This keeps the short-lived VCS token out of the clone URL, out of the stored + // remote in .git/config and out of the git command line (process arguments). + const basicAuth = Buffer.from(`${username}:${vcsToken}`).toString("base64"); + const clonePath = await cloneByGivenURL(new URL(repoUrl), destinationPath, { + ...cloneOptions, + httpExtraHeader: `Authorization: Basic ${basicAuth}`, + }); // Pre-warm Gradle in the background so the student's first build is faster. // The depth (off/daemon/deps/full) is controlled by the GRADLE_PREWARM env var. + // Runs only after the student explicitly confirms (see warmupGradleDaemon). const exercise = getState().displayedExercise; if ((exercise as ProgrammingExercise)?.programmingLanguage === ProgrammingLanguage.JAVA) { - warmupGradleDaemon(clonePath, theiaEnv.GRADLE_PREWARM); + void warmupGradleDaemon(clonePath, theiaEnv.GRADLE_PREWARM); } if (!theiaEnv.THEIA_FLAG) { @@ -134,25 +141,46 @@ export async function cloneByGivenURL( const clonePath = path.join(destinationPath, repoName); const gitForClone = gitClientFactory.simpleGit(destinationPath); + applyAuthHeaderEnv(gitForClone, options?.httpExtraHeader); try { - await gitForClone.clone(cloneUrl.toString(), clonePath, buildCloneOptions(options?.httpExtraHeader)); + await gitForClone.clone(cloneUrl.toString(), clonePath); } catch (e: any) { throw new Error(`Error cloning repository: ${e.message}`); } + await persistAuthHeader(clonePath, options?.httpExtraHeader); + return clonePath; } -// Builds the extra simple-git clone options. When an HTTP header is provided, it is passed as -// `git clone -c http.extraHeader=...`, which authenticates the initial fetch AND persists the -// header into the new repo's .git/config so later in-IDE fetch/push are authenticated too. -// The token is therefore stored in .git/config; this is acceptable since it is short-lived. -function buildCloneOptions(httpExtraHeader?: string): string[] { +// Injects the auth header into the git process via the GIT_CONFIG_* environment variables +// (git >= 2.31). This authenticates the clone WITHOUT placing the token on the command line, +// so it never appears in process arguments (`ps`). The env-provided config is not written to +// the new repo's .git/config, so persistAuthHeader stores it afterwards for later fetch/push. +function applyAuthHeaderEnv(git: ReturnType, httpExtraHeader?: string): void { + if (!httpExtraHeader) { + return; + } + // `.env` replaces the child process environment, so carry over the current environment + // (PATH etc.) and add the single-entry git config override. + git.env({ + ...process.env, + GIT_CONFIG_COUNT: "1", + GIT_CONFIG_KEY_0: "http.extraHeader", + GIT_CONFIG_VALUE_0: httpExtraHeader, + }); +} + +// Persists the auth header into the cloned repo so later in-IDE fetch/push stay authenticated. +// Residual risk: the short-lived token is written to .git/config here. It is, however, no longer +// embedded in the clone URL / stored remote nor exposed in process arguments. +async function persistAuthHeader(repoPath: string, httpExtraHeader?: string): Promise { if (!httpExtraHeader) { - return []; + return; } - return ["-c", `http.extraHeader=${httpExtraHeader}`]; + const git = gitClientFactory.simpleGit(repoPath); + await git.addConfig("http.extraHeader", httpExtraHeader, false, GitConfigScope.local); } async function cloneIntoWorkspaceRoot( @@ -179,10 +207,13 @@ async function cloneIntoWorkspaceRoot( await clearDirectory(workspacePath); const gitForClone = gitClientFactory.simpleGit(workspacePath); + applyAuthHeaderEnv(gitForClone, httpExtraHeader); // Clone into "." so the exercise repository becomes the workspace root in Theia. - await gitForClone.clone(cloneUrl.toString(), ".", buildCloneOptions(httpExtraHeader)); + await gitForClone.clone(cloneUrl.toString(), "."); cloneSucceeded = true; + await persistAuthHeader(workspacePath, httpExtraHeader); + return workspacePath; } catch (e: any) { throw new Error(`Error cloning repository into workspace root: ${e.message}`); diff --git a/src/participation/gradle.service.ts b/src/participation/gradle.service.ts index d980801b..5971a90d 100644 --- a/src/participation/gradle.service.ts +++ b/src/participation/gradle.service.ts @@ -1,6 +1,7 @@ import { spawn } from "child_process"; import * as fs from "fs"; import * as path from "path"; +import * as vscode from "vscode"; import { GradlePrewarmLevel } from "../theia/env-strategy"; /** @@ -18,41 +19,87 @@ const PREWARM_ARGS: Record, string[]> = { }; /** - * Pre-warms the Gradle build in the background after cloning a repository, so the - * student's first build can skip the phases warmed here. Runs detached and never - * blocks the caller. + * Side effects the prewarm performs, grouped behind a seam so tests can assert the security + * gate (no build is ever started without explicit user confirmation) without touching the + * real filesystem, child_process or VS Code. + */ +export type GradlePrewarmEffects = { + gradlewExists: (gradlewPath: string) => boolean; + confirm: (projectPath: string) => Promise; + makeExecutable: (gradlewPath: string) => void; + run: (projectPath: string, args: string[], level: GradlePrewarmLevel) => void; +}; + +/** + * Asks the student whether the repository's Gradle wrapper may be executed to prewarm the + * build. The prompt is non-modal and defaults to "no": if the student ignores it nothing runs, + * which keeps the feature opt-in while removing the silent code-execution property. + */ +async function promptPrewarmConfirmation(_projectPath: string): Promise { + const choice = await vscode.window.showWarningMessage( + "Scorpio can pre-build this exercise with its Gradle wrapper to speed up your first build. " + + "This runs code from the cloned repository. Only continue for exercises you trust.", + "Pre-build now", + ); + return choice === "Pre-build now"; +} + +function spawnGradle(projectPath: string, args: string[], level: GradlePrewarmLevel): void { + const child = spawn("./gradlew", args, { + cwd: projectPath, + detached: true, + stdio: "ignore", + }); + // Detached failures surface asynchronously via the 'error' event, not a throw. + child.on("error", (error) => { + console.warn(`Gradle prewarm (${level}) failed to start: ${error.message}`); + }); + child.unref(); +} + +export const defaultGradlePrewarmEffects: GradlePrewarmEffects = { + gradlewExists: (gradlewPath) => fs.existsSync(gradlewPath), + confirm: promptPrewarmConfirmation, + makeExecutable: (gradlewPath) => fs.chmodSync(gradlewPath, 0o755), + run: spawnGradle, +}; + +/** + * Pre-warms the Gradle build in the background after cloning a repository, so the student's + * first build can skip the phases warmed here. * - * Silently skips if: + * Security: `./gradlew` and the bundled Gradle wrapper jar are repository-controlled, so a + * tampered exercise template could run arbitrary code during prewarm. The wrapper is therefore + * never executed automatically - the student must explicitly confirm first, and confirmation + * defaults to "no". + * + * Silently skips (no prompt) if: * - Prewarming is disabled (`level` is "off") * - Running on Windows (not a supported environment) * - The project does not contain a `gradlew` file (not a Gradle project) */ -export function warmupGradleDaemon( +export async function warmupGradleDaemon( projectPath: string, level: GradlePrewarmLevel = "daemon", -): void { + effects: GradlePrewarmEffects = defaultGradlePrewarmEffects, +): Promise { if (level === "off" || process.platform === "win32") { return; } const gradlewPath = path.join(projectPath, "gradlew"); - if (!fs.existsSync(gradlewPath)) { + if (!effects.gradlewExists(gradlewPath)) { + return; + } + + const confirmed = await effects.confirm(projectPath).catch(() => false); + if (!confirmed) { return; } try { - fs.chmodSync(gradlewPath, 0o755); - - const child = spawn("./gradlew", PREWARM_ARGS[level], { - cwd: projectPath, - detached: true, - stdio: "ignore", - }); - // Detached failures surface asynchronously via the 'error' event, not the throw below. - child.on("error", (error) => { - console.warn(`Gradle prewarm (${level}) failed to start: ${error.message}`); - }); - child.unref(); + effects.makeExecutable(gradlewPath); + effects.run(projectPath, PREWARM_ARGS[level], level); } catch (error: any) { console.warn(`Gradle prewarm (${level}) failed: ${error.message}`); } diff --git a/src/shared/artemis-url.ts b/src/shared/artemis-url.ts new file mode 100644 index 00000000..ea666bd4 --- /dev/null +++ b/src/shared/artemis-url.ts @@ -0,0 +1,45 @@ +/** + * Decides whether an externally supplied Artemis base URL (for example the `ARTEMIS_URL` + * environment variable injected into a Theia session) may be trusted as the API base. + * + * The candidate is only accepted when it is a well-formed http(s) URL whose origin + * (scheme + host + port) matches the origin the extension is already configured for. This + * prevents an attacker-controlled environment from silently repointing the extension - and the + * Bearer token it attaches to every request - at a rogue host. + * + * Returns the trusted URL string when the candidate is allowed, otherwise `undefined`. + */ +export function resolveAllowedArtemisUrl( + candidate: string | undefined, + configuredBaseUrl: string | undefined, +): string | undefined { + if (!candidate) { + return undefined; + } + + let candidateUrl: URL; + try { + candidateUrl = new URL(candidate); + } catch { + return undefined; + } + + if (candidateUrl.protocol !== "http:" && candidateUrl.protocol !== "https:") { + return undefined; + } + + // Without a configured origin to allowlist against we cannot establish trust, so the + // override is rejected rather than accepted blindly. + if (!configuredBaseUrl) { + return undefined; + } + + let configuredUrl: URL; + try { + configuredUrl = new URL(configuredBaseUrl); + } catch { + return undefined; + } + + return candidateUrl.origin === configuredUrl.origin ? candidate : undefined; +} diff --git a/src/shared/settings.ts b/src/shared/settings.ts index b7407967..3d6028cc 100644 --- a/src/shared/settings.ts +++ b/src/shared/settings.ts @@ -1,6 +1,7 @@ import * as vscode from "vscode"; import { authenticationProvider } from "../extension"; import { theiaEnv } from "../theia/theia"; +import { resolveAllowedArtemisUrl } from "./artemis-url"; export type Settings = { base_url: string | undefined; @@ -21,9 +22,19 @@ export function initSettings() { function getSettings(): Settings { let base_url = vscode.workspace.getConfiguration("scorpio").get("artemis.apiBaseUrl"); if (theiaEnv.ARTEMIS_URL) { - const config = vscode.workspace.getConfiguration("scorpio"); - config.update("artemis.apiBaseUrl", theiaEnv.ARTEMIS_URL, vscode.ConfigurationTarget.Global); - base_url = theiaEnv.ARTEMIS_URL; + // Only honor the environment override when it points at the same origin the extension is + // already configured for. An unvalidated ARTEMIS_URL could otherwise redirect every + // authenticated request (and the Bearer token) to an attacker-controlled host. + const trustedUrl = resolveAllowedArtemisUrl(theiaEnv.ARTEMIS_URL, base_url); + if (trustedUrl) { + const config = vscode.workspace.getConfiguration("scorpio"); + config.update("artemis.apiBaseUrl", trustedUrl, vscode.ConfigurationTarget.Global); + base_url = trustedUrl; + } else { + vscode.window.showErrorMessage( + `Ignoring ARTEMIS_URL "${theiaEnv.ARTEMIS_URL}": it does not match the configured Artemis origin${base_url ? ` (${base_url})` : ""}.`, + ); + } } if (!base_url) { diff --git a/src/shared/websocket.ts b/src/shared/websocket.ts index 45237eef..7db2d3e5 100644 --- a/src/shared/websocket.ts +++ b/src/shared/websocket.ts @@ -108,9 +108,10 @@ export class GenericWebSocket { connectHeaders: { Authorization: `Bearer ${session.accessToken}`, }, - debug: function (str: string) { - console.debug(str); - }, + // Security: never forward STOMP frames to the log. The CONNECT frame carries the + // Authorization header (the Artemis JWT), so logging frames leaks the token. Keep this + // a no-op so the access token is never written to the debug console. + debug: () => {}, reconnectDelay: 5000, heartbeatIncoming: 10000, heartbeatOutgoing: 10000, diff --git a/src/test/suite/artemis-url.test.ts b/src/test/suite/artemis-url.test.ts new file mode 100644 index 00000000..dfafab5c --- /dev/null +++ b/src/test/suite/artemis-url.test.ts @@ -0,0 +1,44 @@ +import * as assert from "assert"; + +import { resolveAllowedArtemisUrl } from "../../shared/artemis-url"; + +suite("resolveAllowedArtemisUrl", () => { + const configured = "https://artemis.cit.tum.de"; + + test("accepts a candidate with the same origin as the configured base URL", () => { + assert.strictEqual( + resolveAllowedArtemisUrl("https://artemis.cit.tum.de", configured), + "https://artemis.cit.tum.de", + ); + assert.strictEqual( + resolveAllowedArtemisUrl("https://artemis.cit.tum.de/api", configured), + "https://artemis.cit.tum.de/api", + ); + }); + + test("rejects a different host", () => { + assert.strictEqual(resolveAllowedArtemisUrl("https://evil.example.com", configured), undefined); + }); + + test("rejects a scheme downgrade and a different port", () => { + assert.strictEqual(resolveAllowedArtemisUrl("http://artemis.cit.tum.de", configured), undefined); + assert.strictEqual( + resolveAllowedArtemisUrl("https://artemis.cit.tum.de:8443", configured), + undefined, + ); + }); + + test("rejects non-http(s) and malformed candidates", () => { + assert.strictEqual( + resolveAllowedArtemisUrl("javascript:alert(1)", configured), + undefined, + ); + assert.strictEqual(resolveAllowedArtemisUrl("not a url", configured), undefined); + assert.strictEqual(resolveAllowedArtemisUrl("", configured), undefined); + assert.strictEqual(resolveAllowedArtemisUrl(undefined, configured), undefined); + }); + + test("rejects any candidate when no configured origin is available", () => { + assert.strictEqual(resolveAllowedArtemisUrl("https://artemis.cit.tum.de", undefined), undefined); + }); +}); diff --git a/src/test/suite/cloning.test.ts b/src/test/suite/cloning.test.ts index 7a6443e8..887a92d1 100644 --- a/src/test/suite/cloning.test.ts +++ b/src/test/suite/cloning.test.ts @@ -3,21 +3,35 @@ import * as assert from "assert"; import { cloneByGivenURL, gitClientFactory } from "../../participation/cloning.service"; type CloneCall = { repo: string; local: string; options: unknown }; +type ConfigCall = { key: string; value: string; append: unknown; scope: unknown }; suite("cloneByGivenURL httpExtraHeader handling", () => { const originalSimpleGit = gitClientFactory.simpleGit; let cloneCalls: CloneCall[]; + let configCalls: ConfigCall[]; + let envCalls: Record[]; setup(() => { cloneCalls = []; - // Swap the git factory for a fake that records the arguments passed to clone(). + configCalls = []; + envCalls = []; + // Swap the git factory for a fake that records env(), clone() and addConfig() arguments. gitClientFactory.simpleGit = (() => { - return { + const fake: any = { + env: (env: Record) => { + envCalls.push(env); + return fake; + }, clone: async (repo: string, local: string, options: unknown) => { cloneCalls.push({ repo, local, options }); return ""; }, + addConfig: async (key: string, value: string, append: unknown, scope: unknown) => { + configCalls.push({ key, value, append, scope }); + return ""; + }, }; + return fake; }) as unknown as typeof gitClientFactory.simpleGit; }); @@ -25,24 +39,43 @@ suite("cloneByGivenURL httpExtraHeader handling", () => { gitClientFactory.simpleGit = originalSimpleGit; }); - test("passes -c http.extraHeader when a header is provided", async () => { - await cloneByGivenURL( - new URL("https://gitea.example.com/course/repo.git"), - "/dest", - { httpExtraHeader: "Authorization: Bearer TESTTOKEN" }, - ); + test("injects the auth header via GIT_CONFIG_* env, never on the command line", async () => { + await cloneByGivenURL(new URL("https://gitea.example.com/course/repo.git"), "/dest", { + httpExtraHeader: "Authorization: Bearer TESTTOKEN", + }); assert.strictEqual(cloneCalls.length, 1); - assert.deepStrictEqual(cloneCalls[0].options, [ - "-c", - "http.extraHeader=Authorization: Bearer TESTTOKEN", - ]); + // The token must not be passed as a clone argument (would be visible in `ps`). + assert.deepStrictEqual(cloneCalls[0].options, undefined); + assert.ok( + !JSON.stringify(cloneCalls[0]).includes("TESTTOKEN"), + "clone arguments must not contain the token", + ); + + // The header is supplied through the git process environment instead. + assert.strictEqual(envCalls.length, 1); + assert.strictEqual(envCalls[0].GIT_CONFIG_COUNT, "1"); + assert.strictEqual(envCalls[0].GIT_CONFIG_KEY_0, "http.extraHeader"); + assert.strictEqual(envCalls[0].GIT_CONFIG_VALUE_0, "Authorization: Bearer TESTTOKEN"); + }); + + test("persists the auth header into the cloned repo for later fetch/push", async () => { + await cloneByGivenURL(new URL("https://gitea.example.com/course/repo.git"), "/dest", { + httpExtraHeader: "Authorization: Bearer TESTTOKEN", + }); + + assert.strictEqual(configCalls.length, 1); + assert.strictEqual(configCalls[0].key, "http.extraHeader"); + assert.strictEqual(configCalls[0].value, "Authorization: Bearer TESTTOKEN"); + assert.strictEqual(configCalls[0].scope, "local"); }); - test("passes no extra config when no header is provided (unchanged path)", async () => { + test("does not touch env or config when no header is provided (unchanged path)", async () => { await cloneByGivenURL(new URL("https://gitea.example.com/course/repo.git"), "/dest"); assert.strictEqual(cloneCalls.length, 1); - assert.deepStrictEqual(cloneCalls[0].options, []); + assert.deepStrictEqual(cloneCalls[0].options, undefined); + assert.strictEqual(envCalls.length, 0); + assert.strictEqual(configCalls.length, 0); }); }); diff --git a/src/test/suite/gradle.test.ts b/src/test/suite/gradle.test.ts new file mode 100644 index 00000000..164e5c51 --- /dev/null +++ b/src/test/suite/gradle.test.ts @@ -0,0 +1,81 @@ +import * as assert from "assert"; + +import { GradlePrewarmEffects, warmupGradleDaemon } from "../../participation/gradle.service"; + +type RunCall = { projectPath: string; args: string[]; level: string }; + +function makeEffects(overrides: Partial) { + const runCalls: RunCall[] = []; + const effects: GradlePrewarmEffects = { + gradlewExists: () => true, + confirm: async () => true, + makeExecutable: () => {}, + run: (projectPath, args, level) => { + runCalls.push({ projectPath, args, level }); + }, + ...overrides, + }; + return { effects, runCalls }; +} + +suite("warmupGradleDaemon security gate", () => { + test("does not run the Gradle wrapper when the user declines", async () => { + const { effects, runCalls } = makeEffects({ confirm: async () => false }); + + await warmupGradleDaemon("/repo", "daemon", effects); + + assert.strictEqual(runCalls.length, 0, "no build may start without confirmation"); + }); + + test("runs the Gradle wrapper only after explicit confirmation", async () => { + const { effects, runCalls } = makeEffects({ confirm: async () => true }); + + await warmupGradleDaemon("/repo", "daemon", effects); + + assert.strictEqual(runCalls.length, 1); + assert.deepStrictEqual(runCalls[0].args, ["--daemon", "help"]); + }); + + test("never prompts or runs when prewarming is disabled", async () => { + let confirmed = false; + const { effects, runCalls } = makeEffects({ + confirm: async () => { + confirmed = true; + return true; + }, + }); + + await warmupGradleDaemon("/repo", "off", effects); + + assert.strictEqual(confirmed, false, "disabled prewarm must not prompt"); + assert.strictEqual(runCalls.length, 0); + }); + + test("does not prompt when the project has no gradlew", async () => { + let confirmed = false; + const { effects, runCalls } = makeEffects({ + gradlewExists: () => false, + confirm: async () => { + confirmed = true; + return true; + }, + }); + + await warmupGradleDaemon("/repo", "full", effects); + + assert.strictEqual(confirmed, false, "non-Gradle project must not prompt"); + assert.strictEqual(runCalls.length, 0); + }); + + test("treats a confirmation failure as a decline", async () => { + const { effects, runCalls } = makeEffects({ + confirm: async () => { + throw new Error("prompt failed"); + }, + }); + + await warmupGradleDaemon("/repo", "deps", effects); + + assert.strictEqual(runCalls.length, 0); + }); +}); From 526f920cf80ff531bdd6cbaf600b416cd62e4abb Mon Sep 17 00:00:00 2001 From: Matthias Linhuber Date: Sat, 17 Oct 2026 15:42:19 +0200 Subject: [PATCH 2/2] feat(gradle): make automatic prewarm configurable, default on The security hardening disabled the automatic Gradle prewarm outright, always requiring the student to confirm before the cloned repository's ./gradlew ran. In practice the first build is long and exercise templates are provisioned by a trusted platform, so most deployments want the prewarm to run automatically. Add GRADLE_PREWARM_AUTO_CONFIRM (default true): when true the prewarm runs automatically after clone; set it to false to require explicit confirmation before any repository-controlled Gradle build is executed (the safe choice when templates are not trusted). GRADLE_PREWARM=off still disables prewarm entirely. Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 9 +++-- README_DEVELOPER.md | 2 +- src/participation/cloning.service.ts | 9 +++-- src/participation/gradle.service.ts | 25 ++++++++------ src/test/suite/env-strategy.test.ts | 44 +++++++++++++++++++++++- src/test/suite/gradle.test.ts | 50 +++++++++++++++++----------- src/theia/env-strategy.ts | 31 +++++++++++++++++ src/theia/theia.ts | 12 ++++--- 8 files changed, 142 insertions(+), 40 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7cd078f3..6766f07b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,9 +10,12 @@ Check [Keep a Changelog](http://keepachangelog.com/) for recommendations on how ### Security -- Gradle prewarm no longer runs the cloned repository's `./gradlew` wrapper automatically. The - student must explicitly confirm before any repository-controlled Gradle build is executed, - removing a silent remote-code-execution path from tampered exercise templates. +- Gradle prewarm's execution of the cloned repository's `./gradlew` wrapper is now configurable + via the `GRADLE_PREWARM_AUTO_CONFIRM` environment variable. It defaults to `true` (prewarm runs + automatically, so the long first build is still skipped where exercise templates are provisioned + by a trusted platform). Set it to `false` to require the student to explicitly confirm before any + repository-controlled Gradle build is executed, removing the silent remote-code-execution path + for deployments that do not trust templates. `GRADLE_PREWARM=off` still disables prewarm entirely. - The VCS/Git access token is no longer embedded in the clone URL or passed on the git command line. It is injected through the git process environment (`GIT_CONFIG_*`) during clone and persisted only into `.git/config` for later in-IDE fetch/push. diff --git a/README_DEVELOPER.md b/README_DEVELOPER.md index b240e1ce..9694eb14 100644 --- a/README_DEVELOPER.md +++ b/README_DEVELOPER.md @@ -43,7 +43,7 @@ The webview can NOT communicate with the Artemis API directly due to CORS. There ### Environment variables -At activation the core loads the session environment via a strategy (see [src/theia/env-strategy.ts](src/theia/env-strategy.ts)): from the process environment (lazy sessions) or by polling the data-bridge extension's `dataBridge.getEnvState` command until it reports `injected` (eager / prewarmed sessions). A fixed set of known keys (`ARTEMIS_TOKEN`, `ARTEMIS_URL`, `GIT_URI`, `GIT_USER`, `GIT_MAIL`, `GIT_TOKEN`, `THEIA`, `GRADLE_PREWARM`) drives the bespoke authentication, clone, git-identity, and prewarm logic. Any **other** variable an external system provides is applied to the integrated terminal environment via `context.environmentVariableCollection`, so terminals and terminal-spawned tasks inherit it. Known keys are deliberately kept out of the terminal environment (this also keeps `ARTEMIS_TOKEN` out of shells). Note that `environmentVariableCollection` affects only terminals started after activation, not already-running backend or language-server processes. +At activation the core loads the session environment via a strategy (see [src/theia/env-strategy.ts](src/theia/env-strategy.ts)): from the process environment (lazy sessions) or by polling the data-bridge extension's `dataBridge.getEnvState` command until it reports `injected` (eager / prewarmed sessions). A fixed set of known keys (`ARTEMIS_TOKEN`, `ARTEMIS_URL`, `GIT_URI`, `GIT_USER`, `GIT_MAIL`, `GIT_TOKEN`, `THEIA`, `GRADLE_PREWARM`, `GRADLE_PREWARM_AUTO_CONFIRM`) drives the bespoke authentication, clone, git-identity, and prewarm logic. `GRADLE_PREWARM` selects the prewarm depth (`off`/`daemon`/`deps`/`full`) and `GRADLE_PREWARM_AUTO_CONFIRM` (default `true`) decides whether the prewarm may run automatically or must be confirmed by the student first. Any **other** variable an external system provides is applied to the integrated terminal environment via `context.environmentVariableCollection`, so terminals and terminal-spawned tasks inherit it. Known keys are deliberately kept out of the terminal environment (this also keeps `ARTEMIS_TOKEN` out of shells). Note that `environmentVariableCollection` affects only terminals started after activation, not already-running backend or language-server processes. ### Webview diff --git a/src/participation/cloning.service.ts b/src/participation/cloning.service.ts index 10d1a76a..ff73b569 100644 --- a/src/participation/cloning.service.ts +++ b/src/participation/cloning.service.ts @@ -91,10 +91,15 @@ export async function cloneUserRepo(repoUrl: string, username: string) { // Pre-warm Gradle in the background so the student's first build is faster. // The depth (off/daemon/deps/full) is controlled by the GRADLE_PREWARM env var. - // Runs only after the student explicitly confirms (see warmupGradleDaemon). + // By default this runs automatically; set GRADLE_PREWARM_AUTO_CONFIRM=false to + // require the student to confirm first (see warmupGradleDaemon). const exercise = getState().displayedExercise; if ((exercise as ProgrammingExercise)?.programmingLanguage === ProgrammingLanguage.JAVA) { - void warmupGradleDaemon(clonePath, theiaEnv.GRADLE_PREWARM); + void warmupGradleDaemon( + clonePath, + theiaEnv.GRADLE_PREWARM, + theiaEnv.GRADLE_PREWARM_AUTO_CONFIRM, + ); } if (!theiaEnv.THEIA_FLAG) { diff --git a/src/participation/gradle.service.ts b/src/participation/gradle.service.ts index 5971a90d..3f8ccb37 100644 --- a/src/participation/gradle.service.ts +++ b/src/participation/gradle.service.ts @@ -19,9 +19,9 @@ const PREWARM_ARGS: Record, string[]> = { }; /** - * Side effects the prewarm performs, grouped behind a seam so tests can assert the security - * gate (no build is ever started without explicit user confirmation) without touching the - * real filesystem, child_process or VS Code. + * Side effects the prewarm performs, grouped behind a seam so tests can assert the confirmation + * gate (when it is active, no build starts without the student's confirmation) without touching + * the real filesystem, child_process or VS Code. */ export type GradlePrewarmEffects = { gradlewExists: (gradlewPath: string) => boolean; @@ -32,8 +32,9 @@ export type GradlePrewarmEffects = { /** * Asks the student whether the repository's Gradle wrapper may be executed to prewarm the - * build. The prompt is non-modal and defaults to "no": if the student ignores it nothing runs, - * which keeps the feature opt-in while removing the silent code-execution property. + * build. Used only when GRADLE_PREWARM_AUTO_CONFIRM is false. The prompt is non-modal and + * defaults to "no": if the student ignores it nothing runs, removing the silent code-execution + * property for deployments that do not trust exercise templates. */ async function promptPrewarmConfirmation(_projectPath: string): Promise { const choice = await vscode.window.showWarningMessage( @@ -69,11 +70,14 @@ export const defaultGradlePrewarmEffects: GradlePrewarmEffects = { * first build can skip the phases warmed here. * * Security: `./gradlew` and the bundled Gradle wrapper jar are repository-controlled, so a - * tampered exercise template could run arbitrary code during prewarm. The wrapper is therefore - * never executed automatically - the student must explicitly confirm first, and confirmation - * defaults to "no". + * tampered exercise template could run arbitrary code during prewarm. The behavior is governed + * by `autoConfirm` (the GRADLE_PREWARM_AUTO_CONFIRM env var): + * - `true` (default): the prewarm runs automatically. Appropriate when exercise templates are + * provisioned by a trusted platform, where the long first build matters more than the risk. + * - `false`: the student must explicitly confirm before the wrapper runs, and confirmation + * defaults to "no". The safe choice when templates are not trusted. * - * Silently skips (no prompt) if: + * Silently skips (no prompt, no build) if: * - Prewarming is disabled (`level` is "off") * - Running on Windows (not a supported environment) * - The project does not contain a `gradlew` file (not a Gradle project) @@ -81,6 +85,7 @@ export const defaultGradlePrewarmEffects: GradlePrewarmEffects = { export async function warmupGradleDaemon( projectPath: string, level: GradlePrewarmLevel = "daemon", + autoConfirm = true, effects: GradlePrewarmEffects = defaultGradlePrewarmEffects, ): Promise { if (level === "off" || process.platform === "win32") { @@ -92,7 +97,7 @@ export async function warmupGradleDaemon( return; } - const confirmed = await effects.confirm(projectPath).catch(() => false); + const confirmed = autoConfirm ? true : await effects.confirm(projectPath).catch(() => false); if (!confirmed) { return; } diff --git a/src/test/suite/env-strategy.test.ts b/src/test/suite/env-strategy.test.ts index bc38d67c..ebc19ad3 100644 --- a/src/test/suite/env-strategy.test.ts +++ b/src/test/suite/env-strategy.test.ts @@ -1,6 +1,10 @@ import * as assert from "assert"; -import { KNOWN_ENV_KEYS, parseTheiaEnv } from "../../theia/env-strategy"; +import { + DEFAULT_GRADLE_PREWARM_AUTO_CONFIRM, + KNOWN_ENV_KEYS, + parseTheiaEnv, +} from "../../theia/env-strategy"; suite("env-strategy GIT_TOKEN handling", () => { test("parseTheiaEnv returns GIT_TOKEN when present", () => { @@ -45,3 +49,41 @@ suite("env-strategy GIT_TOKEN handling", () => { assert.strictEqual(parsed.GRADLE_PREWARM, "daemon"); }); }); + +suite("env-strategy GRADLE_PREWARM_AUTO_CONFIRM handling", () => { + function parse(value: string | undefined) { + return parseTheiaEnv({ + THEIA: "1", + GRADLE_PREWARM: "daemon", + GRADLE_PREWARM_AUTO_CONFIRM: value, + }).GRADLE_PREWARM_AUTO_CONFIRM; + } + + test("defaults to automatic prewarm (true) when unset", () => { + assert.strictEqual(DEFAULT_GRADLE_PREWARM_AUTO_CONFIRM, true); + assert.strictEqual(parse(undefined), true); + }); + + test("is queried by the env strategy", () => { + assert.ok( + KNOWN_ENV_KEYS.has("GRADLE_PREWARM_AUTO_CONFIRM"), + "GRADLE_PREWARM_AUTO_CONFIRM must be among the queried keys", + ); + }); + + test("parses falsy values as false (require confirmation)", () => { + for (const value of ["false", "0", "no", "off", "FALSE", " Off "]) { + assert.strictEqual(parse(value), false, `"${value}" should disable auto-confirm`); + } + }); + + test("parses truthy values as true", () => { + for (const value of ["true", "1", "yes", "on", "TRUE"]) { + assert.strictEqual(parse(value), true, `"${value}" should enable auto-confirm`); + } + }); + + test("falls back to the default on an unrecognized value", () => { + assert.strictEqual(parse("maybe"), DEFAULT_GRADLE_PREWARM_AUTO_CONFIRM); + }); +}); diff --git a/src/test/suite/gradle.test.ts b/src/test/suite/gradle.test.ts index 164e5c51..d03547aa 100644 --- a/src/test/suite/gradle.test.ts +++ b/src/test/suite/gradle.test.ts @@ -18,11 +18,11 @@ function makeEffects(overrides: Partial) { return { effects, runCalls }; } -suite("warmupGradleDaemon security gate", () => { +suite("warmupGradleDaemon confirmation gate (GRADLE_PREWARM_AUTO_CONFIRM=false)", () => { test("does not run the Gradle wrapper when the user declines", async () => { const { effects, runCalls } = makeEffects({ confirm: async () => false }); - await warmupGradleDaemon("/repo", "daemon", effects); + await warmupGradleDaemon("/repo", "daemon", false, effects); assert.strictEqual(runCalls.length, 0, "no build may start without confirmation"); }); @@ -30,52 +30,64 @@ suite("warmupGradleDaemon security gate", () => { test("runs the Gradle wrapper only after explicit confirmation", async () => { const { effects, runCalls } = makeEffects({ confirm: async () => true }); - await warmupGradleDaemon("/repo", "daemon", effects); + await warmupGradleDaemon("/repo", "daemon", false, effects); assert.strictEqual(runCalls.length, 1); assert.deepStrictEqual(runCalls[0].args, ["--daemon", "help"]); }); - test("never prompts or runs when prewarming is disabled", async () => { - let confirmed = false; + test("treats a confirmation failure as a decline", async () => { const { effects, runCalls } = makeEffects({ confirm: async () => { - confirmed = true; - return true; + throw new Error("prompt failed"); }, }); - await warmupGradleDaemon("/repo", "off", effects); + await warmupGradleDaemon("/repo", "deps", false, effects); - assert.strictEqual(confirmed, false, "disabled prewarm must not prompt"); assert.strictEqual(runCalls.length, 0); }); +}); - test("does not prompt when the project has no gradlew", async () => { - let confirmed = false; +suite("warmupGradleDaemon auto-confirm (GRADLE_PREWARM_AUTO_CONFIRM=true, default)", () => { + test("runs the Gradle wrapper without prompting", async () => { + let prompted = false; const { effects, runCalls } = makeEffects({ - gradlewExists: () => false, confirm: async () => { - confirmed = true; + prompted = true; return true; }, }); - await warmupGradleDaemon("/repo", "full", effects); + await warmupGradleDaemon("/repo", "full", true, effects); - assert.strictEqual(confirmed, false, "non-Gradle project must not prompt"); - assert.strictEqual(runCalls.length, 0); + assert.strictEqual(prompted, false, "auto-confirm must not prompt the student"); + assert.strictEqual(runCalls.length, 1); + assert.deepStrictEqual(runCalls[0].args, ["--daemon", "build", "-x", "test"]); }); +}); - test("treats a confirmation failure as a decline", async () => { +suite("warmupGradleDaemon skip conditions", () => { + test("never prompts or runs when prewarming is disabled", async () => { + let prompted = false; const { effects, runCalls } = makeEffects({ confirm: async () => { - throw new Error("prompt failed"); + prompted = true; + return true; }, }); - await warmupGradleDaemon("/repo", "deps", effects); + await warmupGradleDaemon("/repo", "off", false, effects); + assert.strictEqual(prompted, false, "disabled prewarm must not prompt"); assert.strictEqual(runCalls.length, 0); }); + + test("does not run when the project has no gradlew, even with auto-confirm", async () => { + const { effects, runCalls } = makeEffects({ gradlewExists: () => false }); + + await warmupGradleDaemon("/repo", "full", true, effects); + + assert.strictEqual(runCalls.length, 0, "non-Gradle project must not build"); + }); }); diff --git a/src/theia/env-strategy.ts b/src/theia/env-strategy.ts index aae96b8f..6431236f 100644 --- a/src/theia/env-strategy.ts +++ b/src/theia/env-strategy.ts @@ -20,6 +20,7 @@ export type TheiaEnv = { GIT_MAIL: string | undefined; GIT_TOKEN: string | undefined; GRADLE_PREWARM: GradlePrewarmLevel; + GRADLE_PREWARM_AUTO_CONFIRM: boolean; }; const ENV_KEYS = [ @@ -31,6 +32,7 @@ const ENV_KEYS = [ "GIT_MAIL", "GIT_TOKEN", "GRADLE_PREWARM", + "GRADLE_PREWARM_AUTO_CONFIRM", ] as const satisfies Array; /** @@ -39,6 +41,17 @@ const ENV_KEYS = [ */ export const DEFAULT_GRADLE_PREWARM: GradlePrewarmLevel = "daemon"; +/** + * Whether the Gradle prewarm may run automatically after cloning, without asking + * the student first. Default true: exercise templates are provisioned by the + * platform and prewarming saves a long first build, so most deployments want it + * on. Set the GRADLE_PREWARM_AUTO_CONFIRM env var to false to require an explicit + * confirmation before any repository-controlled `gradlew` is executed (the safe + * choice when templates are not trusted). GRADLE_PREWARM="off" disables prewarm + * entirely regardless of this flag. + */ +export const DEFAULT_GRADLE_PREWARM_AUTO_CONFIRM = true; + function parseGradlePrewarm(value: string | undefined): GradlePrewarmLevel { const level = value?.trim().toLowerCase(); if (level === "off" || level === "daemon" || level === "deps" || level === "full") { @@ -52,6 +65,23 @@ function parseGradlePrewarm(value: string | undefined): GradlePrewarmLevel { return DEFAULT_GRADLE_PREWARM; } +function parseGradlePrewarmAutoConfirm(value: string | undefined): boolean { + const flag = value?.trim().toLowerCase(); + if (flag === undefined || flag === "") { + return DEFAULT_GRADLE_PREWARM_AUTO_CONFIRM; + } + if (flag === "true" || flag === "1" || flag === "yes" || flag === "on") { + return true; + } + if (flag === "false" || flag === "0" || flag === "no" || flag === "off") { + return false; + } + console.warn( + `Unknown GRADLE_PREWARM_AUTO_CONFIRM value "${value}", falling back to "${DEFAULT_GRADLE_PREWARM_AUTO_CONFIRM}"`, + ); + return DEFAULT_GRADLE_PREWARM_AUTO_CONFIRM; +} + /** * Result of loading the environment: the typed `env` (the known keys with their * bespoke handling) plus `raw`, the full untyped map of every variable that was @@ -81,6 +111,7 @@ export function parseTheiaEnv(env: Record): TheiaEnv GIT_MAIL: env["GIT_MAIL"], GIT_TOKEN: env["GIT_TOKEN"], GRADLE_PREWARM: parseGradlePrewarm(env["GRADLE_PREWARM"]), + GRADLE_PREWARM_AUTO_CONFIRM: parseGradlePrewarmAutoConfirm(env["GRADLE_PREWARM_AUTO_CONFIRM"]), }; } diff --git a/src/theia/theia.ts b/src/theia/theia.ts index 20880be6..2138785a 100644 --- a/src/theia/theia.ts +++ b/src/theia/theia.ts @@ -2,7 +2,12 @@ import * as vscode from "vscode"; import simpleGit, { GitConfigScope } from "simple-git"; import { hostname } from "os"; import { cloneByGivenURL } from "../participation/cloning.service"; -import { createTheiaEnvStrategy, DEFAULT_GRADLE_PREWARM, TheiaEnv } from "./env-strategy"; +import { + createTheiaEnvStrategy, + DEFAULT_GRADLE_PREWARM, + DEFAULT_GRADLE_PREWARM_AUTO_CONFIRM, + TheiaEnv, +} from "./env-strategy"; // Mutable theiaEnv that gets populated after loading export let theiaEnv: TheiaEnv = { @@ -14,6 +19,7 @@ export let theiaEnv: TheiaEnv = { GIT_MAIL: undefined, GIT_TOKEN: undefined, GRADLE_PREWARM: DEFAULT_GRADLE_PREWARM, + GRADLE_PREWARM_AUTO_CONFIRM: DEFAULT_GRADLE_PREWARM_AUTO_CONFIRM, }; // Full untyped map of every variable that was provided (including arbitrary keys @@ -55,9 +61,7 @@ export async function initTheia() { await cloneByGivenURL(theiaEnv.GIT_URI, workspaceFolderUri.fsPath, { mode: "workspace-root", - httpExtraHeader: useGiteaToken - ? `Authorization: Bearer ${theiaEnv.GIT_TOKEN}` - : undefined, + httpExtraHeader: useGiteaToken ? `Authorization: Bearer ${theiaEnv.GIT_TOKEN}` : undefined, }); }