Repository navigation
fix(dev): deny writes to git config and pin the wrapper's PATH - #87021
Conversation
|
😎 Merged successfully - details. |
🤖 CI report🚨 Trunk lane — universal laneThis PR is assigned to the universal lane. It cannot merge in parallel with other PRs, so it can take longer to merge. Ask dev-ex if you think this is wrong.
|
| File | Size | Δ vs base |
|---|---|---|
render-query/src/render-query/render-query.js |
21.67 MiB | 🔺 +9.2 KiB (+0.0%) |
exporter/src/queries/schema.js |
1.16 MiB | 🔺 +9.0 KiB (+0.8%) |
posthog-app/src/queries/schema.js |
1.16 MiB | 🔺 +9.0 KiB (+0.8%) |
posthog-app/_parent/products/engineering_analytics/frontend/scenes/EngineeringAnalyticsScene.js |
61.8 KiB | 🔺 +3.2 KiB (+5.4%) |
posthog-app/_parent/products/signals/frontend/inbox/InboxScene.js |
704.0 KiB | 🔺 +1.5 KiB (+0.2%) |
Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report
✅ Eager graph — within budget
How much code each root ships on the eager path — downloaded and parsed before the surface is interactive. Measured from the esbuild output chunks (post-tree-shake, static imports only); lazy import() / React.lazy chunks are not counted.
| Root | Eager (shipped) | Δ vs base | Budget |
|---|---|---|---|
entry (logged-out pages, app bootstrap)src/index.tsx |
1.36 MiB · 22 files | 🔺 +211 B (+0.0%) | ███░░░░░░░ 30.2% of 4.51 MiB |
authenticated shell (every logged-in page)src/scenes/AuthenticatedShell.tsx |
8.77 MiB · 3,235 files | 🔺 +130 B (+0.0%) | █████████░ 90.2% of 9.71 MiB |
🟢 node_modules/monaco-editor/ stays out of src/index.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 node_modules/monaco-editor/ stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
Largest files eagerly shipped from src/index.tsx
| Size | File |
|---|---|
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 24.6 KiB | ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js |
| 6.3 KiB | ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js |
| 4.5 KiB | ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js |
| 3.9 KiB | ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js |
| 1.4 KiB | ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js |
| 1.3 KiB | src/RootErrorBoundary.tsx |
| 912 B | ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js |
| 789 B | src/scenes/ChunkLoadErrorBoundary.tsx |
| 762 B | src/index.tsx |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 306.5 KiB | ../node_modules/.pnpm/posthog-js@1.418.17/node_modules/posthog-js/dist/rrweb.js |
| 267.7 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 258.5 KiB | ../node_modules/.pnpm/posthog-js@1.418.17/node_modules/posthog-js/dist/module.js |
| 246.0 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 154.2 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 104.6 KiB | src/lib/api.ts |
| 95.1 KiB | ../packages/quill/packages/quill/dist/index.js |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 90.6 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479
✅ Toolbar bundle — eager 2.25 MiB within budget
What the toolbar ships to customer pages, measured from the esbuild output (minified, post-tree-shake). The eager set is the entry plus everything statically imported from it — fetched before any feature runs; deferred chunks load lazily. The eager guardrail is 5.72 MiB. Each output file must also stay below 10 MB, where CloudFront stops compressing it. The module boundary is enforced separately by check-toolbar-graph.
| Metric | Size | Δ vs base | Budget |
|---|---|---|---|
| Eager (shipped) entry + static imports |
2.25 MiB · 17 files | 🔺 +211 B (+0.0%) | ████░░░░░░ 39.3% of 5.72 MiB |
| Deferred (lazy) | 2.09 MiB · 33 files | no change | n/a — loads on demand |
Loader dist/toolbar.js |
1.1 KiB | no change | █░░░░░░░░░ 5.8% of 19.5 KiB |
Largest eagerly-shipped chunks
| Size | File |
|---|---|
| 745.9 KiB | dist/toolbar/toolbar-app-QDEUOQFO.css |
| 582.9 KiB | dist/toolbar/chunk-chunk-YRYIO5AK.js |
| 484.6 KiB | dist/toolbar/chunk-chunk-7TYJBGPX.js |
| 133.8 KiB | dist/toolbar/chunk-chunk-HUJ6YVZZ.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-T5KY5WYR.js |
| 71.2 KiB | dist/toolbar/toolbar-app-2USHGT3U.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-27JL52RE.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-5I6DF2DA.js |
| 20.9 KiB | dist/toolbar/chunk-chunk-WRHMR2RW.js |
| 12.2 KiB | dist/toolbar/chunk-chunk-PIK3PADE.js |
Posted automatically by check-toolbar-size · sizes are toolbar output bytes (shipped, post-tree-shake) from the esbuild metafile
✅ Dist folder size — 🔺 +325.4 KiB (+0.0%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 1460.50 MiB · 🔺 +325.4 KiB (+0.0%)
93b4591 to
0c2efc1
Compare
|
✅ Visual changes approved by @haacked — baseline updated in 10 changed. |
🦔 ReviewHog reviewed this pull requestFound 0 must fix, 1 should fix, 2 consider. Published 3 findings (view the review). Resolved comments: 4 declined, 8 left for you |
|
ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
d9c3eed to
1bb35b6
Compare
Sandboxed dependency code could set core.hooksPath or core.fsmonitor in a git config file and have that command run later in the developer's own terminal, outside Seatbelt. Close the write routes that reach it. The profile denies the repo's git config files, the .git node, Homebrew's system gitconfig and the directory holding it, .husky, .flox/env, and the main repo's bin/. The wrapper resolves at render time the physical paths a worktree or a symlinked .git needs, since Seatbelt matches physical paths, and adds the git dir's commondir, which repoints git at another common dir carrying its own config and hooks. One shared predicate decides whether a path is safe to render inside an SBPL literal. The wrapper pins its own PATH to the system directories before the sandbox exists and runs /usr/bin/sandbox-exec by absolute path, so sandboxed code cannot plant a binary that the wrapper then executes unsandboxed. core.hooksPath and blame.ignoreRevsFile are seeded from the flox activation, which runs unsandboxed. husky reports success even when its git config write fails, so without the seed a fresh clone would install no hooks and say nothing. The seed reads the local scope, so a global core.hooksPath does not suppress it. .husky/_/husky.sh stays writable because husky rewrites it on every install and throws if it cannot; the checked-in hooks are protected, and the self-test pins that trade-off so closing it cannot break installs silently. Sibling worktrees and [include]-chained config files stay open, tracked in #79300. Generated-By: PostHog Desktop Task-Id: 7ad0d5f8-020a-4917-8108-a1d66b8ac2c2
1bb35b6 to
6597f14
Compare
|
✅ Security review complete — 3 findings posted as a review, in 11 min. Re-request a |
There was a problem hiding this comment.
Pull request overview
Hardens the macOS dev sandbox wrapper and SBPL profile to prevent sandboxed dependency code from persisting malicious Git configuration or poisoning pre-sandbox binary resolution, while adding self-tests and moving repo-local Git config seeding to flox activation.
Changes:
- Write-deny additional Git config surfaces (
.git/config,.gitredirect file,config.worktree, and Homebrew’s system gitconfig paths) and expand execute-later write-denies in the SBPL profile. - Pin the wrapper’s pre-sandbox
PATHto system directories and invokesandbox-execvia absolute path to prevent PATH poisoning. - Extend
bin/dev-sandbox-selftestwith new regression checks and update flox activation to seed repo-local Git settings that postinstall can’t write under the sandbox.
Reviewed changes
Copilot reviewed 1 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
bin/dev-sandbox.sb |
Adds new write-denies for git config and other execute-later targets, plus a narrow re-allow for Husky’s generated _ dir. |
bin/dev-sandbox-selftest |
Adds new checks for denied git config writes, worktree redirect handling, private TMPDIR profile checks, and PATH passthrough validation. |
bin/dev-sandbox |
Pins pre-sandbox PATH, uses absolute /usr/bin/sandbox-exec, and adds runtime write-denies for physical gitdir config paths. |
.flox/env/on-activate.sh |
Moves repo-local Git config seeding (ignore revs + hooksPath) to flox activation to avoid sandbox write-denies. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Reviews (1): Last reviewed commit: "fix(dev): deny git-config and hook write..." | Re-trigger Greptile |
|
This is security-sensitive sandboxing tooling, and ReviewHog's inline comment on bin/dev-sandbox flags a real, confirmed gap — the shebang still resolves bash through the untrusted inherited PATH before the new PATH pin takes effect, undermining the very escape this PR closes. The bot explicitly declined to auto-fix it because it changes the sandbox mechanism itself and asked a human to confirm the approach; that thread is still unresolved on the current head.
Gate mechanics and policy version
|
|
Reviews (2): Last reviewed commit: "fix(dev): deny git-config and hook write..." | Re-trigger Greptile |
|
Reviews (3): Last reviewed commit: "fix(dev): deny git-config and hook write..." | Re-trigger Greptile |
The shebang resolved bash through the caller's PATH, which keeps repo dirs sandboxed code can write, so a planted bash ran as the developer before the wrapper's own PATH pin could take effect. It is a fixed /bin/bash now. The profile denied Homebrew's etc/gitconfig but left bin/, sbin/ and Cellar/ writable. Those are developer-owned and on PATH, so planting a binary there reaches unsandboxed execution without the git-config indirection. Two self-test cases cover them: a bash planted ahead on PATH must not run, and the Homebrew bin directories must refuse a write. Both were checked red against the previous behavior. Write-side gaps that survive these denies, the git-dir and repo-root node swaps, the activation PATH, and the shared profile cache, are in #87967. Generated-By: PostHog Desktop Task-Id: 7ad0d5f8-020a-4917-8108-a1d66b8ac2c2
feliperalmeida
left a comment
There was a problem hiding this comment.
Other than my inline comment, LGTM. Requesting changes to include the missing subpath unless there's a strong reason against it.
| (subpath "/opt/homebrew/Cellar") | ||
| (subpath "/usr/local/bin") | ||
| (subpath "/usr/local/sbin") | ||
| (subpath "/usr/local/Cellar") |
There was a problem hiding this comment.
In my machine, /opt/homebrew/bin/npm resolves to ../Cellar/node/26.7.0/bin/npm and then /opt/homebrew/lib/node_modules/npm/bin/npm-cli.js.
The deny covers bin and Cellar but misses lib. We should add it, unless there's a reason not to.
There was a problem hiding this comment.
Good catch! Added lib.
bin/npm resolves through Cellar to lib/node_modules/npm/bin/npm-cli.js, and Seatbelt matches resolved paths, so the bin/Cellar denies alone left the executed file writable. Generated-By: PostHog Desktop Task-Id: ab2f1c37-3023-4f06-ab9d-d5ba99ac5b16
gantoine
left a comment
There was a problem hiding this comment.
security team to decide is this makes sense
Generated-By: PostHog Desktop Task-Id: ab2f1c37-3023-4f06-ab9d-d5ba99ac5b16
Problem
A malicious dependency in a sandboxed dev service can run code on the developer's Mac outside the sandbox, with full privileges. Two routes reach that. Neither depends on the sandbox's read rules.
.git/hooks, because hooks run outside Seatbelt. It leaves git's config files writable.core.hooksPathrelocates the hook directory those denies protect, and git runscore.fsmonitoron ordinary read commands. A dependency appends two lines, and the developer's nextgit statusruns its command.node_modules/.bin, which sandboxed code can write. A binary planted on one run executes as the developer on the next.Changes
Nothing looks different. Every change removes access, except the last, which moves one setup step.
Git config, all reached by the first route:
.git/configand.git/config.worktreefor both repo roots..gitnode itself. In a worktree that node is a file holdinggitdir: <path>. Rewriting it repoints git at a directory the attacker controls, which brings its own config.PROJECT-relative rule reaches it. This repo setsextensions.worktreeConfig, so the case is live here.PATH, the second route:
/usr/bin/sandbox-execby absolute path. The finalexechands the child's PATH toenv, and that PATH keeps the repo's writable bins. A bare name let sandboxed code plant the binary that creates the sandbox.One setup step moves:
blame.ignoreRevsFile, whichpackage.json's postinstall did before. That script runs inside the sandbox, so the write failed behind its own|| trueand a fresh clone never got the setting. Apnpm installoutside flox no longer seeds it.Denying a
.gitnode leaves entries inside a real.gitdirectory alone, so ordinary git writes still work. Cargo's git-dependency writes go to~/.cargo/git/db.Note
Git config files reached through
[include]stay writable, and stay unreadable inside the sandbox. The read failure breaks cargo builds withcould not stat '~/.gitconfig...': Operation not permitted. The write gap leaves the first route open through a file this profile does not name. Both need one mechanism: enumerate the files git reports, instead of listing them by hand. #79300 holds that work.How did you test this code?
bin/dev-sandbox-selftestgains four cases. Each one was verified to fail with its own deny reverted..git/config. No existing case covered it..gitredirect file. The case builds a throwaway worktree instead of probing this checkout. Appending to a directory fails on its own, so on a plain clone the probe would report success without the sandbox being involved.config.worktree.Ran locally on macOS 26.6. Checked by hand there:
git status,git log,git rev-parseand a repo write still work inside the sandbox.sed,mktemp,cat,mv,rm,touch,cut,unameordirnameno longer runs during the pre-sandbox phase..gitis a symlink.blame.ignoreRevsFileon a fresh repo and leaves an existing value alone.Check 12 now renders into a private
TMPDIR. Globbing the shared one turned up profiles left by other worktrees, which this sandbox does not deny. Clearing those evicted a cache those worktrees were still using.Not checked: Linux, where the wrapper exits early and does nothing. CI runs the self-test on
depot-macos-15.Automatic notifications
Docs update
None. No user-facing behavior or documented workflow changes.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Claude Code (Opus 5) in PostHog Desktop. Skills invoked:
/security-review,/writing-pr-descriptions,/address-pr-reviews,/squash,/simplify,/explain-open,/followup.This began as one branch. That branch also taught the sandbox to follow
[include]chains in the developer's git config, to fix the cargo failure noted above. Adversarial review rounds kept finding escapes in that half. Three of those were introduced by earlier fixes in the same session. So the work was split. This PR carries only the half that removes access and adds no new capability. The discovery half stays in #79300 for redesign.The
.gitnode andconfig.worktreedenies came from probing, not from a report. Both are live on this repo, which uses worktrees withextensions.worktreeConfig.A later cleanup pass probed every file
git config --list --show-originreports. That found the writable Homebrew gitconfig, the baresandbox-exec, and the postinstall whose config write the new denies had started swallowing. It also measured the[include]gap the note above records. Those files look protected on this machine only because they are symlinks into a read-denied directory. A plain file in their place is writable.Created with PostHog Desktop