ci: harden release-bot token handling and e2e temp-dir squatting - #1433
Conversation
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.
…/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.
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.
|
Warning Review limit reached
More reviews will be available in 30 minutes and 15 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR hardens the release workflow token setup and adds filesystem security checks around Obsidian E2E profile directories, wiring the checks into startup, stop, and CLI flows. New tests cover both the workflow configuration and the secure-directory helpers. ChangesRelease Workflow Token Hardening
E2E Profile Directory Security Guards
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
chhoumann
changed the title
Deploying quickadd with
|
| Latest commit: |
b833688
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d0ae5b9e.quickadd.pages.dev |
| Branch Preview URL: | https://chhoumann-sec-dev-tooling-ha.quickadd.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/release-workflow-hardening.test.ts (1)
56-63: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert the permission list is closed, not just present.
Line 61-63 would still pass if a future edit adds another
permission-*scope to the App token step. Since the test says these three are the complete set, assert the exact set.Proposed test tightening
- 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); + const permissionLines = appToken + .split("\n") + .map((line) => line.trim()) + .filter((line) => line.startsWith("permission-")); + + expect(permissionLines.sort()).toEqual( + [ + "permission-contents: write", + "permission-issues: write", + "permission-pull-requests: write", + ].sort(), + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/release-workflow-hardening.test.ts` around lines 56 - 63, The App token permission test is only checking that three permission-* scopes are present, so it would still pass if extra scopes are added. Tighten the assertion in the release workflow test around stepContaining("uses: actions/create-github-app-token@") to verify the exact closed set of permissions, so only contents, issues, and pull-requests with write access are allowed and any additional permission scope causes a failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/start-obsidian-e2e-instance.mjs`:
- Around line 305-308: Secure the parent profile root before validating the
instance path in launch-only flows by mirroring the ordering used in
prepareObsidianProfile. In launchObsidianInstance, call ensureSecureDir on
options.profileRoot before the existing ensureSecureDir(options.instancePath)
check so a future relaunch path cannot leave a writable parent around a
validated child. Keep the guard in place for both paths and preserve the
existing temp-squat protection logic.
---
Nitpick comments:
In `@tests/release-workflow-hardening.test.ts`:
- Around line 56-63: The App token permission test is only checking that three
permission-* scopes are present, so it would still pass if extra scopes are
added. Tighten the assertion in the release workflow test around
stepContaining("uses: actions/create-github-app-token@") to verify the exact
closed set of permissions, so only contents, issues, and pull-requests with
write access are allowed and any additional permission scope causes a failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eb2b5db9-250b-4b37-ab9f-c980be1c84e1
📒 Files selected for processing (6)
.github/workflows/release.ymlscripts/obsidian-e2e-cli.mjsscripts/start-obsidian-e2e-instance.mjsscripts/stop-obsidian-e2e-instance.mjstests/release-workflow-hardening.test.tstests/start-obsidian-e2e-instance.test.ts
…stance 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.
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.
|
🎉 This PR is included in version 2.14.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
1 participant
Summary
Hardens two MEDIUM dev/CI hygiene findings from the security scan. Both are defense-in-depth for the build/release toolchain, not user-facing plugin behavior, and both are assessed honestly against QuickAdd's threat model below. No legitimate local/dev functionality is weakened - the live Obsidian e2e harness still launches and
quickadd:listreturnsok:true, and the release push path is unchanged.Finding 1 - Release-bot token persisted to disk before untrusted install/build/test (
.github/workflows/release.yml)The gap (real): the Release job mints a GitHub App token - the "QuickAdd Release Bot", a branch-protection bypass actor - and checked out with
persist-credentialsdefaulting totrue, writing the App credential into.git/config(http.extraheader).pnpm install --frozen-lockfile(which runs dependency lifecycle scripts),pnpm run build, andpnpm run testthen run before semantic-release consumes the token, so a compromised dependency's postinstall could read it off disk. The token was also over-scoped:create-github-app-tokenwas called with nopermission-*inputs, so it inherited all of the App's installation permissions. Every other workflow in the repo (ci.yml,codeql.yml,dependency-review.yml) already setspersist-credentials: false- this was the lone gap.Fix:
persist-credentials: falseon the app-token checkout. The token is still passed (checkout fetches as the App), but it is no longer written to.git/config. The push still works: semantic-release authenticates from theGITHUB_TOKENenv on the Release step - it overwritesoptions.repositoryUrlwith a token-embeddedx-access-token:URL (semantic-release/index.js:67+lib/get-git-auth-url.js:69,99-102) and never reads.git/config.permission-contents/issues/pull-requests: write(the complete set semantic-release uses: push commit/tag + create Release viacontents; comment on released issues/PRs viaissues/pull-requests). Attestation, Find-PR, and Comment-on-PR all use the defaultGITHUB_TOKEN, not this token.harden-runneris deliberately left onegress-policy: audit: switching toblockneeds an allowlist of every endpoint a real release touches (GitHub API, codeload, npm registry, asset-upload host, Sigstore) and silently breaks releases if incomplete - it cannot be validated without a real release run, and the disk-exposure window is already closed by (1).Finding 2 - Predictable
/tmpprofile dir holding a symlink to the real keychain (scripts/start-obsidian-e2e-instance.mjs)The gap (low likelihood, real pattern): the e2e profile root defaults to
/tmp/quickadd-obsidian-e2e(world-writable parent) and symlinks the real~/Library/Keychainsinto aHOMEObsidian launches with.fs.mkdir(..., {recursive})is a no-op for ownership/mode on a pre-existing dir, so a co-located actor on a shared/reused host could temp-squat the predictable root before the victim's first run and TOCTOU-hijack the keychain-bearing writes. This is a dev/CI harness on macOS, not the shipped plugin; the common single-user-dev case has no attacker. But the asset (the login keychain) and the recognized insecure-temp pattern justify proportionate hardening.Fix -
ensureSecureDir(fail-closed), not a redesign:lstatfirst so we nevermkdirthrough a pre-existing symlink; create only when absent; then assert the dir is a real directory we own with no group/other access. A symlink, a foreign owner, or a loose mode is rejected (not chmod-repaired - see review note). A0o700-owned dir cannot have foreign-planted children, so descending into it is then safe.profileRootbeforeinstancePath) inprepareObsidianProfile, before the reaper in both mains, inlaunchObsidianInstance, and - validate-only viaassertSecureDirIfPresent- in the stop/reap teardown paths./tmpdefault and the stable instance-id layout are kept. Moving the root underos.tmpdir()was prototyped and reproduced to break the harness:/var/folders/.../Tpushes Obsidian's$HOME/.obsidian-cli.sockpast the macOS 104-bytesun_pathlimit (EINVAL), and vitest wouldn't catch it. The keychain symlink is load-bearing (SecretStorage parity, pinned by tests) and is left intact; the temp-dir guard removes the hijack vector without touching it.Exploit / mitigation evidence (run live in this worktree's isolated vault)
pnpm run obsidian:e2e -- quickadd:listlaunches Obsidian and returns{"ok":true,"command":"quickadd:list","count":0,"choices":[]};pnpm run stop:e2e-obsidianterminates the process tree and removes the instance dir.startandstop --pruneabort withRefusing to use <path>: it is a symlink or not a regular directory., and the symlink target stays empty.semantic-release@25.0.5source that the push URL is built fromGITHUB_TOKEN, independent of.git/config.Regression tests (fail without the fix)
tests/release-workflow-hardening.test.ts- assertspersist-credentials: false, the retainedtoken:line, and the three scopedpermission-*inputs, anchored to real YAML lines with full-line comments stripped (a commented-out guard cannot satisfy it). RED onmaster(all four guard strings absent), GREEN here.tests/start-obsidian-e2e-instance.test.ts-ensureSecureDir/assertSecureDirIfPresent: fresh ->0o700/ours, loose -> rejected, symlink -> rejected (no write-through), foreign uid -> rejected,currentUid:null-> owner-check skipped, absent ->falsewithout creating. The symlink + foreign-uid cases are RED when the guards are removed.Process
Threat-model judgment and fix design were validated by an adversarial reviewer fleet before coding (it caught and rejected the
os.tmpdir()break), and the implemented diff was reviewed by three opposite-model (Codex) reviewers (skeptic/architect/minimalist) whose consensus findings - reject-only over chmod-repair, guard the teardown paths, anchor the YAML test - are folded in.Gates:
tsc -noEmit,eslint .,svelte-check(0 errors), full vitest (3179 passed / 37 skipped) all green.Summary by CodeRabbit
Bug Fixes
Tests