From 39a93abf896e514381cb69e1db9fd61d49e64c66 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Sun, 28 Jun 2026 21:42:35 +0200 Subject: [PATCH 1/5] ci(release): scope release-bot token and stop persisting it to disk The Release job mints a privileged GitHub App token (a branch-protection bypass actor) and checked out WITH persist-credentials defaulting to true, writing the App credential into .git/config. install/build/test then run before semantic-release consumes it, so a compromised dependency's postinstall could read the token off disk. - persist-credentials: false on the app-token checkout. semantic-release authenticates its push from the GITHUB_TOKEN env on the Release step (it builds an x-access-token URL via getGitAuthUrl; it never reads .git/config), so the release push still works. Matches every other workflow in the repo (ci/codeql/dependency-review all set this). - Scope the minted token to permission-contents/issues/pull-requests:write instead of inheriting ALL of the App's installation permissions. - Regression test asserting both guards on release.yml. --- .github/workflows/release.yml | 18 ++++++- tests/release-workflow-hardening.test.ts | 60 ++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 2 deletions(-) create mode 100644 tests/release-workflow-hardening.test.ts diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 77e614bd..5f177d05 100644 --- a/.github/workflows/release.yml +++ b/.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 diff --git a/tests/release-workflow-hardening.test.ts b/tests/release-workflow-hardening.test.ts new file mode 100644 index 00000000..cfe9e530 --- /dev/null +++ b/tests/release-workflow-hardening.test.ts @@ -0,0 +1,60 @@ +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). +function stepBlocks(content: string): string[] { + const blocks: string[] = []; + let current: string[] = []; + for (const line of content.split("\n")) { + 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. + expect(checkout).toContain("token: ${{ steps.app-token.outputs.token }}"); + expect(checkout).toContain("persist-credentials: false"); + }); + + it("scopes the minted App token to least privilege", () => { + const appToken = stepContaining("uses: actions/create-github-app-token@"); + // Without explicit permission-* inputs the token inherits ALL of the App's + // installation permissions. These three are the complete set semantic-release + // uses (push commit/tag + create Release; comment on issues/PRs). + expect(appToken).toContain("permission-contents: write"); + expect(appToken).toContain("permission-issues: write"); + expect(appToken).toContain("permission-pull-requests: write"); + }); +}); From f6ba744629828fff8a39d6f47096fe1ce107cfae Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Sun, 28 Jun 2026 21:45:24 +0200 Subject: [PATCH 2/5] test(e2e): fail closed on a hijacked Obsidian profile dir (temp-squat/TOCTOU) The per-instance Obsidian profile defaults under world-writable /tmp and symlinks the real macOS keychain into a HOME the instance launches with. fs.mkdir(...,{recursive}) is a no-op for ownership/mode on a pre-existing dir, so a co-located actor who pre-creates the predictable profile root (or symlinks it elsewhere) before the victim's first run could redirect the keychain-bearing writes. Add ensureSecureDir: lstat FIRST (never mkdir through a pre-existing symlink), create only when absent, then assert the dir is a real directory we own with no group/other access (tightening a loose mode we own, refusing a foreign owner or a symlink). Validate profileRoot before instancePath (parent-first) in prepareObsidianProfile and before the reaper scans the root in both start + the CLI wrapper. The /tmp sticky bit then prevents swapping our validated 0o700 dir. Keeps the stable instance-id layout and the /tmp default unchanged (moving the root under os.tmpdir() was rejected: /var/folders pushes Obsidian's unix control socket past the 104-byte sun_path limit and breaks the harness). Regression tests cover fresh/loose/symlink/foreign-uid/no-getuid. --- scripts/obsidian-e2e-cli.mjs | 4 ++ scripts/start-obsidian-e2e-instance.mjs | 56 +++++++++++++++++++++ tests/start-obsidian-e2e-instance.test.ts | 61 +++++++++++++++++++++++ 3 files changed, 121 insertions(+) diff --git a/scripts/obsidian-e2e-cli.mjs b/scripts/obsidian-e2e-cli.mjs index e8f75db6..b2d0beea 100644 --- a/scripts/obsidian-e2e-cli.mjs +++ b/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); diff --git a/scripts/start-obsidian-e2e-instance.mjs b/scripts/start-obsidian-e2e-instance.mjs index f304d9cf..d0b456d1 100644 --- a/scripts/start-obsidian-e2e-instance.mjs +++ b/scripts/start-obsidian-e2e-instance.mjs @@ -134,7 +134,60 @@ function safeName(value) { .replace(/^-+|-+$/g, "") || "vault"; } +// Refuse to write our Obsidian profile into a directory we do not exclusively +// 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 the result is a real directory we own +// with no group/other access. 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 = + "currentUid" in options + ? options.currentUid + : typeof process.getuid === "function" + ? process.getuid() + : null; + + 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); + } + + 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}.`, + ); + } + // We own it; if it is group/other-accessible (an older run under a loose + // umask, or another tool) tighten it so the keychain-bearing profile stays + // private. + if ((stat.mode & 0o077) !== 0) { + await fs.chmod(dir, 0o700); + } + + return dir; +} + 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 }); @@ -352,6 +405,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); diff --git a/tests/start-obsidian-e2e-instance.test.ts b/tests/start-obsidian-e2e-instance.test.ts index cb232276..6d43a5e1 100644 --- a/tests/start-obsidian-e2e-instance.test.ts +++ b/tests/start-obsidian-e2e-instance.test.ts @@ -3,6 +3,7 @@ import os from "node:os"; import path from "node:path"; import { afterEach, describe, expect, it } from "vitest"; import { + ensureSecureDir, INSTANCE_MARKER_FILE, parseArgs, prepareObsidianProfile, @@ -104,6 +105,66 @@ 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("tightens a pre-existing group/world-accessible dir we own", async () => { + const root = await makeTempDir("quickadd-secure"); + const dir = path.join(root, "loose"); + await fs.mkdir(dir, { recursive: true }); + await fs.chmod(dir, 0o777); + + await ensureSecureDir(dir); + + expect((await fs.lstat(dir)).mode & 0o077).toBe(0); + }); + + 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); + }); +}); + async function exists(filePath: string) { try { await fs.lstat(filePath); From cd4b696009b4637fef146b65d3f6884478e34b57 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Sun, 28 Jun 2026 22:10:50 +0200 Subject: [PATCH 3/5] test(e2e): close adversarial-review gaps in the temp-squat hardening Three Codex reviewers (skeptic/architect/minimalist) converged on real gaps: - Reject loose (group/other-accessible) pre-existing profile dirs instead of chmod-repairing them. An attacker who could write while the dir was loose may already have planted a 'home' symlink that a parent chmod would not undo (later recursive mkdir / keychain link would follow it). A 0o700-owned dir cannot have foreign-planted children, so reject-only fully closes the race - and drops a branch. - Guard the teardown paths too: stop/reap scanned and removed under the predictable profile root without validation. New assertSecureDirIfPresent (validate-only, never creates/chmods) refuses a hijacked/symlinked root in stop main() and reapOrphanedInstances. - Swap launchObsidianInstance's raw recursive mkdir for ensureSecureDir so a future relaunch path cannot bypass the guard. - Anchor the release.yml regression test to real YAML lines and strip comments, so a commented-out guard can no longer satisfy it. Regression tests updated: loose-dir now asserts rejection; added assertSecureDirIfPresent coverage (absent->false/no-create, symlink->throws, owned->true). Live: harness still launches + QuickAdd ok; stop --prune refuses a symlinked root; full teardown still terminates+removes. --- scripts/start-obsidian-e2e-instance.mjs | 94 +++++++++++++++-------- scripts/stop-obsidian-e2e-instance.mjs | 11 +++ tests/release-workflow-hardening.test.ts | 21 +++-- tests/start-obsidian-e2e-instance.test.ts | 40 +++++++++- 4 files changed, 123 insertions(+), 43 deletions(-) diff --git a/scripts/start-obsidian-e2e-instance.mjs b/scripts/start-obsidian-e2e-instance.mjs index d0b456d1..be9c2470 100644 --- a/scripts/start-obsidian-e2e-instance.mjs +++ b/scripts/start-obsidian-e2e-instance.mjs @@ -134,33 +134,22 @@ function safeName(value) { .replace(/^-+|-+$/g, "") || "vault"; } -// Refuse to write our Obsidian profile into a directory we do not exclusively -// 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 the result is a real directory we own -// with no group/other access. 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 = - "currentUid" in options - ? options.currentUid - : typeof process.getuid === "function" - ? process.getuid() - : null; - - 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); - } +// 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.`, @@ -171,16 +160,56 @@ export async function ensureSecureDir(dir, options = {}) { `Refusing to use ${dir}: owned by uid ${stat.uid}, not ${currentUid}.`, ); } - // We own it; if it is group/other-accessible (an older run under a loose - // umask, or another tool) tighten it so the keychain-bearing profile stays - // private. if ((stat.mode & 0o077) !== 0) { - await fs.chmod(dir, 0o700); + 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 @@ -273,7 +302,10 @@ async function writeJson(filePath, value) { } export async function launchObsidianInstance(options) { - await fs.mkdir(options.instancePath, { recursive: true }); + // Validate (and create when absent) the instance dir here too, so a future + // "relaunch existing instance" path cannot bypass the temp-squat guard that + // prepareObsidianProfile applies in the normal start flow. + await ensureSecureDir(options.instancePath); await execFileAsync("/usr/bin/open", [ "-n", "-g", diff --git a/scripts/stop-obsidian-e2e-instance.mjs b/scripts/stop-obsidian-e2e-instance.mjs index da936b7d..8818ab07 100644 --- a/scripts/stop-obsidian-e2e-instance.mjs +++ b/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, diff --git a/tests/release-workflow-hardening.test.ts b/tests/release-workflow-hardening.test.ts index cfe9e530..e0ac5914 100644 --- a/tests/release-workflow-hardening.test.ts +++ b/tests/release-workflow-hardening.test.ts @@ -16,11 +16,13 @@ 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). +// 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 content.split("\n")) { + for (const line of lines) { if (/^\s*- name:/.test(line)) { if (current.length > 0) blocks.push(current.join("\n")); current = [line]; @@ -43,9 +45,12 @@ describe("release workflow hardening", () => { 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. - expect(checkout).toContain("token: ${{ steps.app-token.outputs.token }}"); - expect(checkout).toContain("persist-credentials: false"); + // 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 least privilege", () => { @@ -53,8 +58,8 @@ describe("release workflow hardening", () => { // Without explicit permission-* inputs the token inherits ALL of the App's // installation permissions. These three are the complete set semantic-release // uses (push commit/tag + create Release; comment on issues/PRs). - expect(appToken).toContain("permission-contents: write"); - expect(appToken).toContain("permission-issues: write"); - expect(appToken).toContain("permission-pull-requests: write"); + expect(appToken).toMatch(/^\s*permission-contents:\s*write\s*$/m); + expect(appToken).toMatch(/^\s*permission-issues:\s*write\s*$/m); + expect(appToken).toMatch(/^\s*permission-pull-requests:\s*write\s*$/m); }); }); diff --git a/tests/start-obsidian-e2e-instance.test.ts b/tests/start-obsidian-e2e-instance.test.ts index 6d43a5e1..cc777681 100644 --- a/tests/start-obsidian-e2e-instance.test.ts +++ b/tests/start-obsidian-e2e-instance.test.ts @@ -3,6 +3,7 @@ import os from "node:os"; import path from "node:path"; import { afterEach, describe, expect, it } from "vitest"; import { + assertSecureDirIfPresent, ensureSecureDir, INSTANCE_MARKER_FILE, parseArgs, @@ -120,15 +121,15 @@ describe("ensureSecureDir", () => { } }); - it("tightens a pre-existing group/world-accessible dir we own", async () => { + 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); - await ensureSecureDir(dir); - - expect((await fs.lstat(dir)).mode & 0o077).toBe(0); + // 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 () => { @@ -165,6 +166,37 @@ describe("ensureSecureDir", () => { }); }); +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); From 53306b69a8bc3c3bd8be67f0b0b3946aef738a50 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Sun, 28 Jun 2026 22:22:56 +0200 Subject: [PATCH 4/5] test(e2e): secure profileRoot before instancePath in launchObsidianInstance CodeRabbit (and the architect reviewer) noted the launch guard validated only the instance dir: a future relaunch path calling launchObsidianInstance without prepareObsidianProfile/main's preflight would leave a loose profileRoot attacker-writable around the validated child. Mirror the parent-first ordering used in prepareObsidianProfile so launch is self-sufficient. --- scripts/start-obsidian-e2e-instance.mjs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/scripts/start-obsidian-e2e-instance.mjs b/scripts/start-obsidian-e2e-instance.mjs index be9c2470..01e64e3b 100644 --- a/scripts/start-obsidian-e2e-instance.mjs +++ b/scripts/start-obsidian-e2e-instance.mjs @@ -302,9 +302,12 @@ async function writeJson(filePath, value) { } export async function launchObsidianInstance(options) { - // Validate (and create when absent) the instance dir here too, so a future - // "relaunch existing instance" path cannot bypass the temp-squat guard that - // prepareObsidianProfile applies in the normal start flow. + // 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", From b83368823efdbe4bfe5e9d29cd99477e0db1c87a Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Sun, 28 Jun 2026 22:27:07 +0200 Subject: [PATCH 5/5] test(release): assert the App-token permission list is closed/exhaustive CodeRabbit nit: the scope test only checked the three expected permission-* inputs were present, so a future over-broad addition (e.g. permission-administration: write) would slip through. Parse every permission-* input on the create-github-app-token step and assert the set equals exactly {contents, issues, pull-requests}: write - nothing broader. Verified: an injected extra permission now fails the test. --- tests/release-workflow-hardening.test.ts | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/tests/release-workflow-hardening.test.ts b/tests/release-workflow-hardening.test.ts index e0ac5914..dee71889 100644 --- a/tests/release-workflow-hardening.test.ts +++ b/tests/release-workflow-hardening.test.ts @@ -53,13 +53,23 @@ describe("release workflow hardening", () => { expect(checkout).toMatch(/^\s*persist-credentials:\s*false\s*$/m); }); - it("scopes the minted App token to least privilege", () => { + 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 = {}; + 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. These three are the complete set semantic-release - // uses (push commit/tag + create Release; comment on issues/PRs). - expect(appToken).toMatch(/^\s*permission-contents:\s*write\s*$/m); - expect(appToken).toMatch(/^\s*permission-issues:\s*write\s*$/m); - expect(appToken).toMatch(/^\s*permission-pull-requests:\s*write\s*$/m); + // 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", + }); }); });