ci: harden release-bot token handling and e2e temp-dir squatting

.github/workflows/release.yml

@@ -48,14 +48,28 @@ jobs:
               with:
                   app-id: ${{ secrets.RELEASE_APP_ID }}
                   private-key: ${{ secrets.RELEASE_APP_PRIVATE_KEY }}
+                  # Least privilege: without these, the minted token inherits ALL of the
+                  # App's installation permissions. semantic-release only needs to push the
+                  # release commit/tag + create the Release (contents) and comment on the
+                  # released issues/PRs (issues/pull-requests). Attestation/Find-PR/Comment
+                  # steps use the default GITHUB_TOKEN, not this token.
+                  permission-contents: write
+                  permission-issues: write
+                  permission-pull-requests: write
 
             - name: Checkout
               uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
               with:
-                  # semantic-release needs full history + tags and the persisted App
-                  # credential to push the release commit. Do not change these.
+                  # semantic-release needs full history + tags to analyze commits.
                   fetch-depth: 0
+                  # The token is still passed so checkout fetches as the App, but we do
+                  # NOT persist it into .git/config: install/build/test run BEFORE the
+                  # release and a compromised dependency could otherwise read the App
+                  # credential off disk. semantic-release authenticates its push from the
+                  # GITHUB_TOKEN env on the Release step (it builds an x-access-token URL,
+                  # it never reads .git/config), so the push still works without it.
                   token: ${{ steps.app-token.outputs.token }}
+                  persist-credentials: false
 
             - name: Setup pnpm
               uses: pnpm/action-setup@0ebf47130e4866e96fce0953f49152a61190b271 # v6.0.9

scripts/obsidian-e2e-cli.mjs

@@ -5,6 +5,7 @@ import process from "node:process";
 import { promisify } from "node:util";
 import { provisionVault } from "./provision-obsidian-e2e-vault.mjs";
 import {
+	ensureSecureDir,
 	launchObsidianInstance,
 	parseArgs as parseInstanceArgs,
 	prepareObsidianProfile,
@@ -170,6 +171,9 @@ async function main() {
 	}
 
 	const options = resolveInstanceOptions(parseInstanceArgs(parsed.instanceArgs));
+	// Validate the profile root we own before the reaper scans/removes anything in
+	// it, so a hijacked root aborts the run loudly instead of being trusted.
+	await ensureSecureDir(options.profileRoot);
 	await reapStaleInstances(options);
 	await ensureObsidianInstance(options);
 	process.exitCode = await spawnObsidian(options, parsed.commandArgs);

scripts/start-obsidian-e2e-instance.mjs

@@ -134,7 +134,89 @@ function safeName(value) {
 		.replace(/^-+|-+$/g, "") || "vault";
 }
 
+// The uid we require every profile directory to be owned by. Injectable so the
+// foreign-owner branch is testable without a second account; null on a platform
+// without process.getuid (none we support — the harness is macOS-only) skips the
+// ownership check rather than comparing against undefined.
+function resolveCurrentUid(options) {
+	if ("currentUid" in options) return options.currentUid;
+	return typeof process.getuid === "function" ? process.getuid() : null;
+}
+
+// Reject any directory we do not exclusively own. A directory we own with no
+// group/other access cannot have a foreign-planted child (only we can write into
+// it), so descending into it later is safe. We REJECT a loose (group/other-
+// accessible) dir rather than chmod-repairing it: an attacker who could write
+// while it was loose may already have planted a `home` symlink that a parent
+// chmod would not undo, and a later recursive mkdir / keychain link would follow.
+function assertOwnedDir(dir, stat, currentUid) {
+	if (stat.isSymbolicLink() || !stat.isDirectory()) {
+		throw new Error(
+			`Refusing to use ${dir}: it is a symlink or not a regular directory.`,
+		);
+	}
+	if (currentUid !== null && stat.uid !== currentUid) {
+		throw new Error(
+			`Refusing to use ${dir}: owned by uid ${stat.uid}, not ${currentUid}.`,
+		);
+	}
+	if ((stat.mode & 0o077) !== 0) {
+		throw new Error(
+			`Refusing to use ${dir}: it is group/other-accessible (mode ${(
+				stat.mode & 0o777
+			).toString(8)}); remove it and retry.`,
+		);
+	}
+}
+
+// Create (when absent) and validate a private profile directory we own. The
+// profile root defaults under world-writable /tmp; if a co-located actor
+// pre-creates it (or symlinks it elsewhere) before our first run,
+// fs.mkdir(..., {recursive}) is a no-op for ownership/mode and we would
+// otherwise write the keychain-bearing HOME (and obsidian.json) through their
+// directory. lstat FIRST so we never mkdir THROUGH a pre-existing symlink, then
+// create only when absent, then assert ownership/mode. Callers must secure a
+// parent before its children so each child is created inside an already-0o700
+// tree the attacker cannot enter (and /tmp's sticky bit then prevents swapping
+// our owned dir entry).
+export async function ensureSecureDir(dir, options = {}) {
+	const currentUid = resolveCurrentUid(options);
+	let stat;
+	try {
+		stat = await fs.lstat(dir);
+	} catch (error) {
+		if (error?.code !== "ENOENT") throw error;
+		await fs.mkdir(dir, { recursive: true, mode: 0o700 });
+		stat = await fs.lstat(dir);
+	}
+	assertOwnedDir(dir, stat, currentUid);
+	return dir;
+}
+
+// Validate an EXISTING profile directory without creating or modifying it.
+// Teardown paths (stop/reap) read and remove inside the root, so they must
+// refuse a hijacked/symlinked root, but must not create one — a missing root
+// just means there is nothing to clean up. Returns false when the path is absent.
+export async function assertSecureDirIfPresent(dir, options = {}) {
+	const currentUid = resolveCurrentUid(options);
+	let stat;
+	try {
+		stat = await fs.lstat(dir);
+	} catch (error) {
+		if (error?.code === "ENOENT") return false;
+		throw error;
+	}
+	assertOwnedDir(dir, stat, currentUid);
+	return true;
+}
+
 export async function prepareObsidianProfile(options) {
+	// Fail closed if our profile tree is not a private directory we own
+	// (temp-squat / TOCTOU guard). Secure the root before the instance dir so the
+	// instance dir is created inside an already-validated 0o700 tree.
+	await ensureSecureDir(options.profileRoot);
+	await ensureSecureDir(options.instancePath);
+
 	const userDataPath = path.join(options.obsidianHome, "Library", "Application Support", "obsidian");
 	await fs.mkdir(userDataPath, { recursive: true, mode: 0o700 });
 	await fs.mkdir(path.join(options.obsidianHome, "Library", "Logs"), { recursive: true, mode: 0o700 });
@@ -220,7 +302,13 @@ async function writeJson(filePath, value) {
 }
 
 export async function launchObsidianInstance(options) {
-	await fs.mkdir(options.instancePath, { recursive: true });
+	// Validate (and create when absent) the profile tree here too, so a future
+	// "relaunch existing instance" path that calls launch without the normal
+	// prepareObsidianProfile/main preflight cannot bypass the temp-squat guard.
+	// Parent-first: a loose profileRoot must not stay attacker-writable around
+	// the validated instance dir.
+	await ensureSecureDir(options.profileRoot);
+	await ensureSecureDir(options.instancePath);
 	await execFileAsync("/usr/bin/open", [
 		"-n",
 		"-g",
@@ -352,6 +440,9 @@ async function main() {
 	}
 
 	const options = resolveInstanceOptions(rawOptions);
+	// Validate the profile root we own before the reaper scans/removes anything in
+	// it, so a hijacked root aborts the run loudly instead of being trusted.
+	await ensureSecureDir(options.profileRoot);
 	await reapStaleInstances(options);
 	const provisionResult = await provisionVault(options);
 	const profileResult = await prepareObsidianProfile(options);

scripts/stop-obsidian-e2e-instance.mjs

@@ -5,6 +5,7 @@ import path from "node:path";
 import process from "node:process";
 import { promisify } from "node:util";
 import {
+	assertSecureDirIfPresent,
 	INSTANCE_MARKER_FILE,
 	resolveInstanceOptions,
 } from "./start-obsidian-e2e-instance.mjs";
@@ -364,6 +365,13 @@ export async function reapOrphanedInstances(options = {}) {
 
 	if (!profileRoot) return { scanned: 0, reaped: [] };
 
+	// Refuse to scan/remove through a hijacked or symlinked profile root (the same
+	// temp-squat guard the start path applies before writing into it). Absent root
+	// => nothing to reap.
+	if (!(await assertSecureDirIfPresent(profileRoot))) {
+		return { scanned: 0, reaped: [] };
+	}
+
 	let entries;
 	try {
 		entries = await readdir(profileRoot);
@@ -416,6 +424,9 @@ async function main() {
 	}
 
 	const options = resolveInstanceOptions(rawOptions);
+	// Refuse to operate inside a hijacked/symlinked profile root before we stop or
+	// remove anything under it.
+	await assertSecureDirIfPresent(options.profileRoot);
 	const summary = await stopInstance(options.instancePath, {
 		dryRun: rawOptions.dryRun,
 		profileRoot: options.profileRoot,

tests/release-workflow-hardening.test.ts

@@ -0,0 +1,75 @@
+import fs from "node:fs";
+import path from "node:path";
+import { fileURLToPath } from "node:url";
+import { describe, expect, it } from "vitest";
+
+// These tests pin the security hardening of the privileged Release workflow:
+// the minted GitHub App token (a branch-protection bypass actor) must be scoped
+// to least privilege and must NOT be persisted into .git/config while untrusted
+// dependency install / build / test code runs ahead of semantic-release. They
+// fail loudly if a future edit regresses either guard. No YAML dependency is
+// available, so we extract the relevant step block textually and assert on it.
+
+const here = path.dirname(fileURLToPath(import.meta.url));
+const releaseYmlPath = path.resolve(here, "..", ".github", "workflows", "release.yml");
+const releaseYml = fs.readFileSync(releaseYmlPath, "utf8");
+
+// Each step in release.yml begins with a `- name:` list item. Split the file
+// into those blocks so an assertion targets exactly one step (and is immune to
+// steps being reordered). Full-line comments are dropped first so a guard that
+// is COMMENTED OUT (e.g. `# persist-credentials: false`) cannot satisfy a check.
+function stepBlocks(content: string): string[] {
+	const lines = content.split("\n").filter((line) => !/^\s*#/.test(line));
+	const blocks: string[] = [];
+	let current: string[] = [];
+	for (const line of lines) {
+		if (/^\s*- name:/.test(line)) {
+			if (current.length > 0) blocks.push(current.join("\n"));
+			current = [line];
+		} else if (current.length > 0) {
+			current.push(line);
+		}
+	}
+	if (current.length > 0) blocks.push(current.join("\n"));
+	return blocks;
+}
+
+function stepContaining(needle: string): string {
+	const matches = stepBlocks(releaseYml).filter((block) => block.includes(needle));
+	expect(matches, `exactly one step should contain ${needle}`).toHaveLength(1);
+	return matches[0];
+}
+
+describe("release workflow hardening", () => {
+	it("does not persist the App credential to disk during install/build/test", () => {
+		const checkout = stepContaining("uses: actions/checkout@");
+		// The token is still passed (semantic-release pushes via the GITHUB_TOKEN env
+		// on the Release step, but checkout fetches as the App); we just refuse to
+		// write it into .git/config where untrusted install/build/test code could read
+		// it. Anchored to real YAML lines so a commented-out guard cannot pass.
+		expect(checkout).toMatch(
+			/^\s*token:\s*\$\{\{\s*steps\.app-token\.outputs\.token\s*\}\}\s*$/m,
+		);
+		expect(checkout).toMatch(/^\s*persist-credentials:\s*false\s*$/m);
+	});
+
+	it("scopes the minted App token to exactly the least-privilege set", () => {
+		const appToken = stepContaining("uses: actions/create-github-app-token@");
+		// Collect every permission-* input declared on the step (comments already
+		// stripped by stepBlocks).
+		const permissions: Record<string, string> = {};
+		for (const line of appToken.split("\n")) {
+			const match = line.match(/^\s*(permission-[a-z-]+):\s*(\S+)\s*$/);
+			if (match) permissions[match[1]] = match[2];
+		}
+		// Without explicit permission-* inputs the token inherits ALL of the App's
+		// installation permissions. Assert the list is CLOSED: exactly these three
+		// scopes (push commit/tag + create Release; comment on issues/PRs) and
+		// nothing broader - a future over-permission addition must fail this test.
+		expect(permissions).toEqual({
+			"permission-contents": "write",
+			"permission-issues": "write",
+			"permission-pull-requests": "write",
+		});
+	});
+});

tests/start-obsidian-e2e-instance.test.ts

@@ -3,6 +3,8 @@ import os from "node:os";
 import path from "node:path";
 import { afterEach, describe, expect, it } from "vitest";
 import {
+	assertSecureDirIfPresent,
+	ensureSecureDir,
 	INSTANCE_MARKER_FILE,
 	parseArgs,
 	prepareObsidianProfile,
@@ -104,6 +106,97 @@ describe("start-obsidian-e2e-instance", () => {
 	});
 });
 
+describe("ensureSecureDir", () => {
+	it("creates a fresh profile dir owned by us with 0o700", async () => {
+		const root = await makeTempDir("quickadd-secure");
+		const dir = path.join(root, "profile-root");
+
+		await ensureSecureDir(dir);
+
+		const stat = await fs.lstat(dir);
+		expect(stat.isDirectory()).toBe(true);
+		expect(stat.mode & 0o777).toBe(0o700);
+		if (typeof process.getuid === "function") {
+			expect(stat.uid).toBe(process.getuid());
+		}
+	});
+
+	it("rejects a pre-existing group/world-accessible dir (planted-child race)", async () => {
+		const root = await makeTempDir("quickadd-secure");
+		const dir = path.join(root, "loose");
+		await fs.mkdir(dir, { recursive: true });
+		await fs.chmod(dir, 0o777);
+
+		// A loose dir may already hold a foreign-planted child symlink that a parent
+		// chmod would not undo, so we refuse it outright rather than "repair" it.
+		await expect(ensureSecureDir(dir)).rejects.toThrow(/group\/other-accessible/);
+	});
+
+	it("refuses a symlink at the profile path (never follows it)", async () => {
+		const root = await makeTempDir("quickadd-secure");
+		const target = path.join(root, "attacker-owned");
+		await fs.mkdir(target, { recursive: true });
+		const link = path.join(root, "profile-root");
+		await fs.symlink(target, link);
+
+		await expect(ensureSecureDir(link)).rejects.toThrow(/symlink or not a regular directory/);
+		// The symlink target must be untouched - we must not have written through it.
+		expect((await fs.readdir(target)).length).toBe(0);
+	});
+
+	it("refuses a dir owned by a different uid (temp-squat)", async () => {
+		const root = await makeTempDir("quickadd-secure");
+		const dir = path.join(root, "foreign");
+		await fs.mkdir(dir, { recursive: true, mode: 0o700 });
+
+		const foreignUid = (process.getuid?.() ?? 0) + 4242;
+		await expect(
+			ensureSecureDir(dir, { currentUid: foreignUid }),
+		).rejects.toThrow(/owned by uid/);
+	});
+
+	it("skips the owner check when getuid is unavailable (currentUid null)", async () => {
+		const root = await makeTempDir("quickadd-secure");
+		const dir = path.join(root, "no-getuid");
+		await fs.mkdir(dir, { recursive: true, mode: 0o700 });
+
+		// Platforms without process.getuid resolve currentUid to null; the owner
+		// check is then skipped rather than throwing against an undefined uid.
+		await expect(ensureSecureDir(dir, { currentUid: null })).resolves.toBe(dir);
+	});
+});
+
+describe("assertSecureDirIfPresent", () => {
+	it("returns false for an absent dir without creating it", async () => {
+		const root = await makeTempDir("quickadd-secure");
+		const dir = path.join(root, "missing");
+
+		await expect(assertSecureDirIfPresent(dir)).resolves.toBe(false);
+		// Teardown must not create the root it is asked to clean up.
+		await expect(fs.lstat(dir)).rejects.toMatchObject({ code: "ENOENT" });
+	});
+
+	it("accepts an existing private dir we own", async () => {
+		const root = await makeTempDir("quickadd-secure");
+		const dir = path.join(root, "ours");
+		await fs.mkdir(dir, { recursive: true, mode: 0o700 });
+
+		await expect(assertSecureDirIfPresent(dir)).resolves.toBe(true);
+	});
+
+	it("refuses a symlinked profile root (teardown must not traverse it)", async () => {
+		const root = await makeTempDir("quickadd-secure");
+		const target = path.join(root, "attacker-owned");
+		await fs.mkdir(target, { recursive: true });
+		const link = path.join(root, "profile-root");
+		await fs.symlink(target, link);
+
+		await expect(assertSecureDirIfPresent(link)).rejects.toThrow(
+			/symlink or not a regular directory/,
+		);
+	});
+});
+
 async function exists(filePath: string) {
 	try {
 		await fs.lstat(filePath);