You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maka.apache.org by "Adarsh-Me (via GitHub)" <gi...@apache.org> on 2026/10/14 18:21:21 UTC
[PR] fix(runtime): resolve a linked worktree read boundary to its common git dir [maka]
Adarsh-Me opened a new pull request, #6105:
URL: https://github.com/apache/maka/pull/6105
## Summary
#4336: in a linked Git worktree, approving the read boundary that the managed Bash tool asks for —
`.git/worktrees/<name>` — still left the repository unreadable, because that admin directory holds
only the worktree's own `HEAD`, index and reflog. Refs, objects, `config` and `info/exclude` live in
the shared directory named by its `commondir` file, so `git status` exited `0` while warning about an
inaccessible `.git/info/exclude` and listed tracked files as additions, and `git log` called the
branch broken.
`normalizeSandboxBoundaryExpansion` now follows that pointer for **read-scoped subtree** entries and
presents the shared git directory instead, so one approval covers the read. Writes keep the exact
path the caller asked for, per the issue's expectation.
Why this location and not the one the issue also names (`builtin-tools.ts:743-749`, cwd as the only
workspace root): both the declared-boundary preflight (`sandbox-boundary-declaration.ts:150`) and the
tool-runtime path (`tool-runtime.ts:2614`) funnel through `normalizeSandboxBoundaryExpansion`, so
normalizing there fixes presentation *and* enforcement in one place, and it is the only point where
"read subtree" and "write subtree" are still distinguishable. I did not touch `normalizeSandboxBoundaryPath`
itself, because `filesystem-executor.ts:282` and `filesystem-worker/client.ts:247` use it for
containment decisions and silently widening those would be wrong. If you would rather this lived in
the workspace-root computation, say so and I will move it.
The `commondir` file is repository-controlled text sitting inside a path the caller asked for, so it
is followed **only** when the target is an existing directory that contains `objects` and `refs`. Any
other pointer leaves the boundary exactly as requested, so a planted worktree cannot turn one read
approval into access to an unrelated directory. Absolute pointers are honoured, as git itself accepts
them.
## Verification
New regressions in `packages/runtime/src/__tests__/sandbox-boundary-declaration.test.ts`, hand-built
fixtures (no git binary needed) covering: the widening, a `write` subtree on the same admin dir staying
exact, a plain directory with no pointer, a pointer aimed at a directory without the git layout, and
an absolute pointer at a real common directory.
```text
$ npm run build -w @maka/core && npm run build -w @maka/runtime # tsc clean, rc=0
$ cd packages/runtime && node --test dist/__tests__/sandbox-boundary-declaration.test.js
ℹ tests 7 ℹ pass 7 ℹ fail 0
# same test file, sandbox-boundary-path.ts reverted to the base commit:
ℹ tests 7 ℹ pass 5 ℹ fail 2
✖ widens a read subtree on a worktree admin dir to the shared git dir
✖ follows an absolute commondir when it names a real common directory
```
The three "must not widen" tests pass on both sides by design — they are guards against the change
over-reaching, not fail-first evidence. The two widening tests fail without the patch.
Neighbour files, A/B'd because they are the ones a widened boundary could plausibly disturb. Base and
head are identical in each case:
- `dist/__tests__/filesystem-authority.test.js`: 12 tests, 10 pass, 1 fail —
`a glob pattern may only leave the search root under a bypass boundary` (`0 !== 1`) fails the same
way on the base commit, so it is not from this change.
- `dist/__tests__/sandbox-boundary-restart-recovery.test.js`: 0/4 on **both** sides. Each failure is
`EBUSY: resource busy or locked, unlink '…\runtime.sqlite-shm'` in teardown — a Windows host limit,
not a behaviour difference.
Not run, and CI should cover them: the whole `@maka/runtime` suite was not compared against base here
(a full `node --test "dist/**/*.test.js"` run hit my 580 s wall at 3931 tests, with 128 failures that
are the same sqlite `EBUSY` class — I did not prove that set against base, so I am not claiming it);
the `@maka/desktop` and `runtime-host` suites; anything macOS- or Linux-only; and the real sandbox
backends, which need the platform Seatbelt/bwrap paths this host does not have. `biome lint` and
`biome format` are clean on both changed files.
## AI use
- [ ] No generative tool made a substantive contribution
- [x] Generative tooling made a substantive contribution
Tool(s) and scope: Qoder (agentic coding environment) drafted the normalization and the five tests,
and ran every command quoted above on this host. I chose the funnel location over the
workspace-root fix, chose the `objects`/`refs` validation as the guard against a hostile pointer,
read the full diff, and trimmed the comments so the library code states the invariant rather than
narrating the issue. The commit carries `Generated-by: Qoder` as CONTRIBUTING.md:36 requires.
## Checklist
- [x] Tests cover the change and fail without it — the two widening tests fail on the base commit
(`2 fail / 5 pass`); the three no-widen guards pass both sides on purpose.
- [x] No new dependencies, no protocol change (the commit hook reported epoch 222 unchanged).
- [x] ASF headers present (the repo's staged audit ran and reported "0 covered / 0 excluded" needed).
- [x] Docs: no sandbox-boundary document enumerates the worktree case, so the convention is stated in
the module (`linkedWorktreeCommonDir` docstring) and in the entry comment. `commondir`
resolution is already described elsewhere in the repo — `docs/architecture/`
`gitoxide-helper-invocation-owner-v1.zh-CN.md:81` covers parsing `.git`/`commondir` path files —
so this does not introduce a new concept, only applies it to boundary approval. Say the word if
a user-facing note belongs somewhere specific.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(runtime): resolve a linked worktree read boundary to its common git dir [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #6105:
URL: https://github.com/apache/maka/pull/6105#discussion_r4302159579
##########
packages/runtime/src/sandbox-boundary-path.ts:
##########
@@ -109,6 +116,65 @@ export async function normalizeSandboxBoundaryExpansion(
return normalized.expansion;
}
+/** `commondir` holds one short relative path; anything larger is not a pointer we will follow. */
+const MAX_GIT_COMMON_DIR_POINTER_CHARS = 1024;
+
+/**
+ * Resolve a linked worktree's admin directory to the repository's shared Git directory.
+ *
+ * `.git/worktrees/<name>` keeps only that worktree's HEAD, index and reflog; refs, objects,
+ * `config` and `info/exclude` live in the directory named by its `commondir` file. A read boundary
+ * on the admin directory alone leaves `git status` and `git log` unable to read the repository
+ * they belong to, so the request is widened to that shared directory before it is approved.
+ *
+ * The pointer is repository-controlled text found inside a path the caller asked for, so it is
+ * followed only when the target really looks like a Git common directory: an existing directory
+ * holding `objects` and `refs` as real directories, reached through no symlink. Anything else
+ * leaves the boundary exactly as requested rather than
+ * widening it into an arbitrary directory.
+ */
+async function linkedWorktreeCommonDir(adminPath: string): Promise<string | undefined> {
+ const pointer = join(adminPath, 'commondir');
+ try {
+ if ((await realTargetTypeFor(pointer)) !== 'file') return undefined;
+ const raw = await fs.readFile(pointer, 'utf8');
+ if (raw.length > MAX_GIT_COMMON_DIR_POINTER_CHARS) return undefined;
+ const target = raw.trim();
+ if (!target || target.includes('\0')) return undefined;
+ const resolved = await realpathAllowMissing(resolve(adminPath, target));
+ return (await isGitCommonDir(resolved)) ? resolved : undefined;
Review Comment:
[P2] This still accepts any real directory that has `objects/` and `refs/` directories. Examples: an unrelated temp directory, an ancestor reached with `../../../..`, a target reached through a symlinked directory (realpath resolves it before the lstat checks), or another repository's `.git`. It also triggers for any read subtree that happens to contain a `commondir` file, not just `.git/worktrees/<name>`. Before substituting, require `realpath(join(resolved, 'worktrees', basename(adminPath))) === adminPath`; otherwise return undefined.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(runtime): resolve a linked worktree read boundary to its common git dir [maka]
Posted by "Adarsh-Me (via GitHub)" <gi...@apache.org>.
Adarsh-Me commented on code in PR #6105:
URL: https://github.com/apache/maka/pull/6105#discussion_r4301960922
##########
packages/runtime/src/sandbox-boundary-path.ts:
##########
@@ -109,6 +116,45 @@ export async function normalizeSandboxBoundaryExpansion(
return normalized.expansion;
}
+/** `commondir` holds one short relative path; anything larger is not a pointer we will follow. */
+const MAX_GIT_COMMON_DIR_POINTER_CHARS = 1024;
+
+/**
+ * Resolve a linked worktree's admin directory to the repository's shared Git directory.
+ *
+ * `.git/worktrees/<name>` keeps only that worktree's HEAD, index and reflog; refs, objects,
+ * `config` and `info/exclude` live in the directory named by its `commondir` file. A read boundary
+ * on the admin directory alone leaves `git status` and `git log` unable to read the repository
+ * they belong to, so the request is widened to that shared directory before it is approved.
+ *
+ * The pointer is repository-controlled text found inside a path the caller asked for, so it is
+ * followed only when the target really looks like a Git common directory: an existing directory
+ * holding `objects` and `refs`. Anything else leaves the boundary exactly as requested rather than
+ * widening it into an arbitrary directory.
+ */
+async function linkedWorktreeCommonDir(adminPath: string): Promise<string | undefined> {
+ const pointer = join(adminPath, 'commondir');
+ try {
+ if ((await targetTypeFor(pointer)) !== 'file') return undefined;
+ const raw = await fs.readFile(pointer, 'utf8');
+ if (raw.length > MAX_GIT_COMMON_DIR_POINTER_CHARS) return undefined;
+ const target = raw.trim();
+ if (!target || target.includes('\0')) return undefined;
+ const resolved = await realpathAllowMissing(resolve(adminPath, target));
+ return (await isGitCommonDir(resolved)) ? resolved : undefined;
+ } catch {
+ return undefined;
+ }
+}
+
+async function isGitCommonDir(path: string): Promise<boolean> {
+ if ((await targetTypeFor(path)) !== 'directory') return false;
+ return (
+ (await targetTypeFor(join(path, 'objects'))) === 'directory' &&
+ (await targetTypeFor(join(path, 'refs'))) !== 'missing'
Review Comment:
Confirmed on my reading: `isGitCommonDir` requires only that `refs` is not `missing`, so a plain `refs` file passes, and every probe uses `fs.stat`, which follows symlinks, so marker and entries can both be symlinks. The rule I'll implement: `commondir` must be a real file, and the target must hold `objects` and `refs` as real directories (lstat).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(runtime): resolve a linked worktree read boundary to its common git dir [maka]
Posted by "Adarsh-Me (via GitHub)" <gi...@apache.org>.
Adarsh-Me commented on code in PR #6105:
URL: https://github.com/apache/maka/pull/6105#discussion_r4304513776
##########
packages/runtime/src/__tests__/sandbox-boundary-declaration.test.ts:
##########
@@ -80,3 +80,289 @@ function toolContext(cwd: string): MakaToolContext {
emitOutput: () => {},
};
}
+
+describe('linked worktree boundary normalization', () => {
+ test('widens a read subtree on a worktree admin dir to the shared git dir', async () => {
+ const repo = await linkedWorktreeRepository('../..');
+ try {
+ const result = await preflightDeclaredSandboxBoundary(
+ {
+ filesystem: {
+ entries: [{ path: repo.adminDir, access: 'read', scope: 'subtree' }],
+ },
+ },
+ toolContext(repo.worktreeDir),
+ );
+
+ assert.deepEqual(result?.filesystem?.entries, [
+ { path: await fs.realpath(join(repo.root, '.git')), access: 'read', scope: 'subtree' },
+ ]);
+ } finally {
+ await fs.rm(repo.root, { recursive: true, force: true });
+ }
+ });
+
+ test('leaves a write subtree on the same admin dir exactly as requested', async () => {
+ const repo = await linkedWorktreeRepository('../..');
+ try {
+ const result = await preflightDeclaredSandboxBoundary(
+ {
+ filesystem: {
+ entries: [{ path: repo.adminDir, access: 'write', scope: 'subtree' }],
+ },
+ },
+ toolContext(repo.worktreeDir),
+ );
+
+ assert.deepEqual(result?.filesystem?.entries, [
+ { path: await fs.realpath(repo.adminDir), access: 'write', scope: 'subtree' },
+ ]);
+ } finally {
+ await fs.rm(repo.root, { recursive: true, force: true });
+ }
+ });
+
+ test('does not touch a read subtree that has no commondir pointer', async () => {
+ const repo = await linkedWorktreeRepository('../..');
+ try {
+ const result = await preflightDeclaredSandboxBoundary(
+ {
+ filesystem: {
+ entries: [{ path: join(repo.root, 'source'), access: 'read', scope: 'subtree' }],
+ },
+ },
+ toolContext(repo.worktreeDir),
+ );
+
+ assert.deepEqual(result?.filesystem?.entries, [
+ { path: await fs.realpath(join(repo.root, 'source')), access: 'read', scope: 'subtree' },
+ ]);
+ } finally {
+ await fs.rm(repo.root, { recursive: true, force: true });
+ }
+ });
+
+ test('refuses to follow a commondir that is not a git common directory', async () => {
+ // The pointer is repository-controlled text, so a worktree planted by someone else must not be
+ // able to turn one read approval into access to an unrelated directory.
+ const repo = await linkedWorktreeRepository('ignored');
+ const secrets = join(repo.root, 'secrets');
+ await fs.mkdir(secrets, { recursive: true });
+ await fs.writeFile(join(secrets, 'id_ed25519'), 'private\n');
+ await fs.writeFile(join(repo.adminDir, 'commondir'), `${await fs.realpath(secrets)}\n`);
+ try {
+ const result = await preflightDeclaredSandboxBoundary(
+ {
+ filesystem: {
+ entries: [{ path: repo.adminDir, access: 'read', scope: 'subtree' }],
+ },
+ },
+ toolContext(repo.worktreeDir),
+ );
+
+ assert.deepEqual(result?.filesystem?.entries, [
+ { path: await fs.realpath(repo.adminDir), access: 'read', scope: 'subtree' },
+ ]);
+ const realSecrets = await fs.realpath(secrets);
+ assert.ok(!JSON.stringify(result).includes(realSecrets));
+ } finally {
+ await fs.rm(repo.root, { recursive: true, force: true });
+ }
+ });
+
+ test('follows an absolute commondir that really owns the admin directory', async () => {
+ const commonDir = await fs.mkdtemp(join(tmpdir(), 'maka-boundary-common-dir-'));
+ await fs.mkdir(join(commonDir, 'objects'), { recursive: true });
+ await fs.mkdir(join(commonDir, 'refs'), { recursive: true });
+ const repo = await worktreeAdminInsideCommonDir(commonDir);
+ try {
+ const result = await preflightDeclaredSandboxBoundary(
+ {
+ filesystem: {
+ entries: [{ path: repo.adminDir, access: 'read', scope: 'subtree' }],
+ },
+ },
+ toolContext(repo.worktreeDir),
+ );
+
+ assert.deepEqual(result?.filesystem?.entries, [
+ { path: await fs.realpath(commonDir), access: 'read', scope: 'subtree' },
+ ]);
+ } finally {
+ await fs.rm(repo.root, { recursive: true, force: true });
+ await fs.rm(commonDir, { recursive: true, force: true });
+ }
+ });
+
+ test('refuses an absolute pointer to a directory that only looks like a common directory', async () => {
+ // A temp directory holding objects/ and refs/ is still not this repository's shared directory:
+ // the admin path the caller declared does not live inside it.
+ const commonDir = await fs.mkdtemp(join(tmpdir(), 'maka-boundary-look-alike-'));
+ await fs.mkdir(join(commonDir, 'objects'), { recursive: true });
+ await fs.mkdir(join(commonDir, 'refs'), { recursive: true });
+ const repo = await linkedWorktreeRepository(await fs.realpath(commonDir));
+ try {
+ const result = await preflightDeclaredSandboxBoundary(
+ {
+ filesystem: {
+ entries: [{ path: repo.adminDir, access: 'read', scope: 'subtree' }],
+ },
+ },
+ toolContext(repo.worktreeDir),
+ );
+
+ assert.deepEqual(result?.filesystem?.entries, [
+ { path: await fs.realpath(repo.adminDir), access: 'read', scope: 'subtree' },
+ ]);
+ assert.ok(!JSON.stringify(result).includes(await fs.realpath(commonDir)));
+ } finally {
+ await fs.rm(repo.root, { recursive: true, force: true });
+ await fs.rm(commonDir, { recursive: true, force: true });
+ }
+ });
+
+ test('refuses an ancestor that looks like a common directory', async () => {
+ const repo = await linkedWorktreeRepository('../../..');
+ await fs.mkdir(join(repo.root, 'objects'), { recursive: true });
+ await fs.mkdir(join(repo.root, 'refs'), { recursive: true });
+ try {
+ const result = await preflightDeclaredSandboxBoundary(
+ {
+ filesystem: {
+ entries: [{ path: repo.adminDir, access: 'read', scope: 'subtree' }],
+ },
+ },
+ toolContext(repo.worktreeDir),
+ );
+
+ assert.deepEqual(result?.filesystem?.entries, [
+ { path: await fs.realpath(repo.adminDir), access: 'read', scope: 'subtree' },
+ ]);
+ } finally {
+ await fs.rm(repo.root, { recursive: true, force: true });
+ }
+ });
+
+ test('refuses a commondir pointer that is a symlink', async () => {
+ // The pointer has to be the marker itself: a symlink is repository text, not proof that this
+ // worktree keeps a commondir.
+ const repo = await linkedWorktreeRepository('ignored');
+ const realPointer = join(repo.root, 'stashed-commondir');
+ await fs.writeFile(realPointer, '../..\n');
+ await fs.rm(join(repo.adminDir, 'commondir'));
+ await fs.symlink(await fs.realpath(realPointer), join(repo.adminDir, 'commondir'));
+ try {
+ const result = await preflightDeclaredSandboxBoundary(
+ {
+ filesystem: {
+ entries: [{ path: repo.adminDir, access: 'read', scope: 'subtree' }],
+ },
+ },
+ toolContext(repo.worktreeDir),
+ );
+
+ assert.deepEqual(result?.filesystem?.entries, [
+ { path: await fs.realpath(repo.adminDir), access: 'read', scope: 'subtree' },
+ ]);
+ } finally {
+ await fs.rm(repo.root, { recursive: true, force: true });
+ }
+ });
+
+ test('refuses a candidate whose refs entry is a plain file', async () => {
+ const fake = await fs.mkdtemp(join(tmpdir(), 'maka-boundary-file-refs-'));
+ await fs.mkdir(join(fake, 'objects'), { recursive: true });
+ await fs.writeFile(join(fake, 'refs'), 'not-a-directory\n');
+ const repo = await worktreeAdminInsideCommonDir(fake);
+ try {
+ const result = await preflightDeclaredSandboxBoundary(
+ {
+ filesystem: {
+ entries: [{ path: repo.adminDir, access: 'read', scope: 'subtree' }],
+ },
+ },
+ toolContext(repo.worktreeDir),
+ );
+
+ assert.deepEqual(result?.filesystem?.entries, [
+ { path: await fs.realpath(repo.adminDir), access: 'read', scope: 'subtree' },
+ ]);
+ assert.ok(!JSON.stringify(result).includes(await fs.realpath(fake)));
Review Comment:
Dropped on head `9a0782bfb` — `deepEqual` already proves no widening, as you said. It passed locally because Windows gives different path forms for `realpath(fake)` and the entry, so the substring never matched; on Linux it is a prefix, hence the red `test`. My miss, not CI's. 12/12 here, `biome lint`/`format` clean.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(runtime): resolve a linked worktree read boundary to its common git dir [maka]
Posted by "Adarsh-Me (via GitHub)" <gi...@apache.org>.
Adarsh-Me commented on code in PR #6105:
URL: https://github.com/apache/maka/pull/6105#discussion_r4301975986
##########
packages/runtime/src/sandbox-boundary-path.ts:
##########
@@ -109,6 +116,45 @@ export async function normalizeSandboxBoundaryExpansion(
return normalized.expansion;
}
+/** `commondir` holds one short relative path; anything larger is not a pointer we will follow. */
+const MAX_GIT_COMMON_DIR_POINTER_CHARS = 1024;
+
+/**
+ * Resolve a linked worktree's admin directory to the repository's shared Git directory.
+ *
+ * `.git/worktrees/<name>` keeps only that worktree's HEAD, index and reflog; refs, objects,
+ * `config` and `info/exclude` live in the directory named by its `commondir` file. A read boundary
+ * on the admin directory alone leaves `git status` and `git log` unable to read the repository
+ * they belong to, so the request is widened to that shared directory before it is approved.
+ *
+ * The pointer is repository-controlled text found inside a path the caller asked for, so it is
+ * followed only when the target really looks like a Git common directory: an existing directory
+ * holding `objects` and `refs`. Anything else leaves the boundary exactly as requested rather than
+ * widening it into an arbitrary directory.
+ */
+async function linkedWorktreeCommonDir(adminPath: string): Promise<string | undefined> {
+ const pointer = join(adminPath, 'commondir');
+ try {
+ if ((await targetTypeFor(pointer)) !== 'file') return undefined;
+ const raw = await fs.readFile(pointer, 'utf8');
+ if (raw.length > MAX_GIT_COMMON_DIR_POINTER_CHARS) return undefined;
+ const target = raw.trim();
+ if (!target || target.includes('\0')) return undefined;
+ const resolved = await realpathAllowMissing(resolve(adminPath, target));
+ return (await isGitCommonDir(resolved)) ? resolved : undefined;
+ } catch {
+ return undefined;
+ }
+}
+
+async function isGitCommonDir(path: string): Promise<boolean> {
+ if ((await targetTypeFor(path)) !== 'directory') return false;
+ return (
+ (await targetTypeFor(join(path, 'objects'))) === 'directory' &&
+ (await targetTypeFor(join(path, 'refs'))) !== 'missing'
Review Comment:
Implemented on head `4af28176d`: `realTargetTypeFor` (lstat) now guards the marker and both qualifying entries, so `objects` and `refs` must be real directories. Fail-first here: your three shapes failed 3/10 against the previous source and pass 10/10 on this head; `biome lint` and `format` clean. The symlinked-`.git/objects` trade-off is written into the body.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(runtime): resolve a linked worktree read boundary to its common git dir [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #6105:
URL: https://github.com/apache/maka/pull/6105#discussion_r4275202608
##########
packages/runtime/src/sandbox-boundary-path.ts:
##########
@@ -109,6 +116,45 @@ export async function normalizeSandboxBoundaryExpansion(
return normalized.expansion;
}
+/** `commondir` holds one short relative path; anything larger is not a pointer we will follow. */
+const MAX_GIT_COMMON_DIR_POINTER_CHARS = 1024;
+
+/**
+ * Resolve a linked worktree's admin directory to the repository's shared Git directory.
+ *
+ * `.git/worktrees/<name>` keeps only that worktree's HEAD, index and reflog; refs, objects,
+ * `config` and `info/exclude` live in the directory named by its `commondir` file. A read boundary
+ * on the admin directory alone leaves `git status` and `git log` unable to read the repository
+ * they belong to, so the request is widened to that shared directory before it is approved.
+ *
+ * The pointer is repository-controlled text found inside a path the caller asked for, so it is
+ * followed only when the target really looks like a Git common directory: an existing directory
+ * holding `objects` and `refs`. Anything else leaves the boundary exactly as requested rather than
+ * widening it into an arbitrary directory.
+ */
+async function linkedWorktreeCommonDir(adminPath: string): Promise<string | undefined> {
+ const pointer = join(adminPath, 'commondir');
+ try {
+ if ((await targetTypeFor(pointer)) !== 'file') return undefined;
+ const raw = await fs.readFile(pointer, 'utf8');
+ if (raw.length > MAX_GIT_COMMON_DIR_POINTER_CHARS) return undefined;
+ const target = raw.trim();
+ if (!target || target.includes('\0')) return undefined;
+ const resolved = await realpathAllowMissing(resolve(adminPath, target));
+ return (await isGitCommonDir(resolved)) ? resolved : undefined;
+ } catch {
+ return undefined;
+ }
+}
+
+async function isGitCommonDir(path: string): Promise<boolean> {
+ if ((await targetTypeFor(path)) !== 'directory') return false;
+ return (
+ (await targetTypeFor(join(path, 'objects'))) === 'directory' &&
+ (await targetTypeFor(join(path, 'refs'))) !== 'missing'
Review Comment:
[P2] Verify that the common directory belongs to the requested worktree
This accepts an unrelated directory with an empty objects directory and even a plain refs file; fs.stat also accepts symlinked marker entries. A commondir pointing there then replaces the requested admin read subtree with read access to the entire unrelated directory. Absolute/relative pointers and symlink variants reproduce this with real files, while the base implementation keeps the requested path. The altered target is shown before approval, so this is not an approval bypass, but untrusted repository text is still choosing the authorization target without proving a worktree relationship. Require a canonical relationship such as commonDir/worktrees/name == requested admin for the normal layout, otherwise leave the boundary unchanged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(runtime): resolve a linked worktree read boundary to its common git dir [maka]
Posted by "Adarsh-Me (via GitHub)" <gi...@apache.org>.
Adarsh-Me commented on code in PR #6105:
URL: https://github.com/apache/maka/pull/6105#discussion_r4302248301
##########
packages/runtime/src/sandbox-boundary-path.ts:
##########
@@ -109,6 +116,65 @@ export async function normalizeSandboxBoundaryExpansion(
return normalized.expansion;
}
+/** `commondir` holds one short relative path; anything larger is not a pointer we will follow. */
+const MAX_GIT_COMMON_DIR_POINTER_CHARS = 1024;
+
+/**
+ * Resolve a linked worktree's admin directory to the repository's shared Git directory.
+ *
+ * `.git/worktrees/<name>` keeps only that worktree's HEAD, index and reflog; refs, objects,
+ * `config` and `info/exclude` live in the directory named by its `commondir` file. A read boundary
+ * on the admin directory alone leaves `git status` and `git log` unable to read the repository
+ * they belong to, so the request is widened to that shared directory before it is approved.
+ *
+ * The pointer is repository-controlled text found inside a path the caller asked for, so it is
+ * followed only when the target really looks like a Git common directory: an existing directory
+ * holding `objects` and `refs` as real directories, reached through no symlink. Anything else
+ * leaves the boundary exactly as requested rather than
+ * widening it into an arbitrary directory.
+ */
+async function linkedWorktreeCommonDir(adminPath: string): Promise<string | undefined> {
+ const pointer = join(adminPath, 'commondir');
+ try {
+ if ((await realTargetTypeFor(pointer)) !== 'file') return undefined;
+ const raw = await fs.readFile(pointer, 'utf8');
+ if (raw.length > MAX_GIT_COMMON_DIR_POINTER_CHARS) return undefined;
+ const target = raw.trim();
+ if (!target || target.includes('\0')) return undefined;
+ const resolved = await realpathAllowMissing(resolve(adminPath, target));
+ return (await isGitCommonDir(resolved)) ? resolved : undefined;
Review Comment:
Implemented on head `59a34531a`: the widening now also requires `realpath(<resolved>/worktrees/<basename(adminPath)>) === realpath(adminPath)`, compared after realpath on both sides — so a temp look-alike, a `../../..` ancestor, another repo's `.git`, and symlink-indirect targets are refused. Fail-first: those 2 shapes fail 10/12 on `4af28176d` and pass 12/12 here; `biome lint`/`format` clean.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(runtime): resolve a linked worktree read boundary to its common git dir [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han merged PR #6105:
URL: https://github.com/apache/maka/pull/6105
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(runtime): resolve a linked worktree read boundary to its common git dir [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on PR #6105:
URL: https://github.com/apache/maka/pull/6105#issuecomment-6256923730
Thanks for the contribution and for addressing the review feedback. Merging now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(runtime): resolve a linked worktree read boundary to its common git dir [maka]
Posted by "Adarsh-Me (via GitHub)" <gi...@apache.org>.
Adarsh-Me commented on PR #6105:
URL: https://github.com/apache/maka/pull/6105#issuecomment-6247966406
@Astro-Han the P3 is real: a UNC `commondir` string reaches `resolve`/`realpathAllowMissing`, so the target is probed before approval. My guard would reject `\` and `//` pointer text as a pure string check, ahead of any filesystem call. Holding it because that path is not fail-first-able or cross-platform deterministic here, and CI is Linux-only. Fold it in, or split it out?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(runtime): resolve a linked worktree read boundary to its common git dir [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #6105:
URL: https://github.com/apache/maka/pull/6105#discussion_r4302452174
##########
packages/runtime/src/__tests__/sandbox-boundary-declaration.test.ts:
##########
@@ -80,3 +80,289 @@ function toolContext(cwd: string): MakaToolContext {
emitOutput: () => {},
};
}
+
+describe('linked worktree boundary normalization', () => {
+ test('widens a read subtree on a worktree admin dir to the shared git dir', async () => {
+ const repo = await linkedWorktreeRepository('../..');
+ try {
+ const result = await preflightDeclaredSandboxBoundary(
+ {
+ filesystem: {
+ entries: [{ path: repo.adminDir, access: 'read', scope: 'subtree' }],
+ },
+ },
+ toolContext(repo.worktreeDir),
+ );
+
+ assert.deepEqual(result?.filesystem?.entries, [
+ { path: await fs.realpath(join(repo.root, '.git')), access: 'read', scope: 'subtree' },
+ ]);
+ } finally {
+ await fs.rm(repo.root, { recursive: true, force: true });
+ }
+ });
+
+ test('leaves a write subtree on the same admin dir exactly as requested', async () => {
+ const repo = await linkedWorktreeRepository('../..');
+ try {
+ const result = await preflightDeclaredSandboxBoundary(
+ {
+ filesystem: {
+ entries: [{ path: repo.adminDir, access: 'write', scope: 'subtree' }],
+ },
+ },
+ toolContext(repo.worktreeDir),
+ );
+
+ assert.deepEqual(result?.filesystem?.entries, [
+ { path: await fs.realpath(repo.adminDir), access: 'write', scope: 'subtree' },
+ ]);
+ } finally {
+ await fs.rm(repo.root, { recursive: true, force: true });
+ }
+ });
+
+ test('does not touch a read subtree that has no commondir pointer', async () => {
+ const repo = await linkedWorktreeRepository('../..');
+ try {
+ const result = await preflightDeclaredSandboxBoundary(
+ {
+ filesystem: {
+ entries: [{ path: join(repo.root, 'source'), access: 'read', scope: 'subtree' }],
+ },
+ },
+ toolContext(repo.worktreeDir),
+ );
+
+ assert.deepEqual(result?.filesystem?.entries, [
+ { path: await fs.realpath(join(repo.root, 'source')), access: 'read', scope: 'subtree' },
+ ]);
+ } finally {
+ await fs.rm(repo.root, { recursive: true, force: true });
+ }
+ });
+
+ test('refuses to follow a commondir that is not a git common directory', async () => {
+ // The pointer is repository-controlled text, so a worktree planted by someone else must not be
+ // able to turn one read approval into access to an unrelated directory.
+ const repo = await linkedWorktreeRepository('ignored');
+ const secrets = join(repo.root, 'secrets');
+ await fs.mkdir(secrets, { recursive: true });
+ await fs.writeFile(join(secrets, 'id_ed25519'), 'private\n');
+ await fs.writeFile(join(repo.adminDir, 'commondir'), `${await fs.realpath(secrets)}\n`);
+ try {
+ const result = await preflightDeclaredSandboxBoundary(
+ {
+ filesystem: {
+ entries: [{ path: repo.adminDir, access: 'read', scope: 'subtree' }],
+ },
+ },
+ toolContext(repo.worktreeDir),
+ );
+
+ assert.deepEqual(result?.filesystem?.entries, [
+ { path: await fs.realpath(repo.adminDir), access: 'read', scope: 'subtree' },
+ ]);
+ const realSecrets = await fs.realpath(secrets);
+ assert.ok(!JSON.stringify(result).includes(realSecrets));
+ } finally {
+ await fs.rm(repo.root, { recursive: true, force: true });
+ }
+ });
+
+ test('follows an absolute commondir that really owns the admin directory', async () => {
+ const commonDir = await fs.mkdtemp(join(tmpdir(), 'maka-boundary-common-dir-'));
+ await fs.mkdir(join(commonDir, 'objects'), { recursive: true });
+ await fs.mkdir(join(commonDir, 'refs'), { recursive: true });
+ const repo = await worktreeAdminInsideCommonDir(commonDir);
+ try {
+ const result = await preflightDeclaredSandboxBoundary(
+ {
+ filesystem: {
+ entries: [{ path: repo.adminDir, access: 'read', scope: 'subtree' }],
+ },
+ },
+ toolContext(repo.worktreeDir),
+ );
+
+ assert.deepEqual(result?.filesystem?.entries, [
+ { path: await fs.realpath(commonDir), access: 'read', scope: 'subtree' },
+ ]);
+ } finally {
+ await fs.rm(repo.root, { recursive: true, force: true });
+ await fs.rm(commonDir, { recursive: true, force: true });
+ }
+ });
+
+ test('refuses an absolute pointer to a directory that only looks like a common directory', async () => {
+ // A temp directory holding objects/ and refs/ is still not this repository's shared directory:
+ // the admin path the caller declared does not live inside it.
+ const commonDir = await fs.mkdtemp(join(tmpdir(), 'maka-boundary-look-alike-'));
+ await fs.mkdir(join(commonDir, 'objects'), { recursive: true });
+ await fs.mkdir(join(commonDir, 'refs'), { recursive: true });
+ const repo = await linkedWorktreeRepository(await fs.realpath(commonDir));
+ try {
+ const result = await preflightDeclaredSandboxBoundary(
+ {
+ filesystem: {
+ entries: [{ path: repo.adminDir, access: 'read', scope: 'subtree' }],
+ },
+ },
+ toolContext(repo.worktreeDir),
+ );
+
+ assert.deepEqual(result?.filesystem?.entries, [
+ { path: await fs.realpath(repo.adminDir), access: 'read', scope: 'subtree' },
+ ]);
+ assert.ok(!JSON.stringify(result).includes(await fs.realpath(commonDir)));
+ } finally {
+ await fs.rm(repo.root, { recursive: true, force: true });
+ await fs.rm(commonDir, { recursive: true, force: true });
+ }
+ });
+
+ test('refuses an ancestor that looks like a common directory', async () => {
+ const repo = await linkedWorktreeRepository('../../..');
+ await fs.mkdir(join(repo.root, 'objects'), { recursive: true });
+ await fs.mkdir(join(repo.root, 'refs'), { recursive: true });
+ try {
+ const result = await preflightDeclaredSandboxBoundary(
+ {
+ filesystem: {
+ entries: [{ path: repo.adminDir, access: 'read', scope: 'subtree' }],
+ },
+ },
+ toolContext(repo.worktreeDir),
+ );
+
+ assert.deepEqual(result?.filesystem?.entries, [
+ { path: await fs.realpath(repo.adminDir), access: 'read', scope: 'subtree' },
+ ]);
+ } finally {
+ await fs.rm(repo.root, { recursive: true, force: true });
+ }
+ });
+
+ test('refuses a commondir pointer that is a symlink', async () => {
+ // The pointer has to be the marker itself: a symlink is repository text, not proof that this
+ // worktree keeps a commondir.
+ const repo = await linkedWorktreeRepository('ignored');
+ const realPointer = join(repo.root, 'stashed-commondir');
+ await fs.writeFile(realPointer, '../..\n');
+ await fs.rm(join(repo.adminDir, 'commondir'));
+ await fs.symlink(await fs.realpath(realPointer), join(repo.adminDir, 'commondir'));
+ try {
+ const result = await preflightDeclaredSandboxBoundary(
+ {
+ filesystem: {
+ entries: [{ path: repo.adminDir, access: 'read', scope: 'subtree' }],
+ },
+ },
+ toolContext(repo.worktreeDir),
+ );
+
+ assert.deepEqual(result?.filesystem?.entries, [
+ { path: await fs.realpath(repo.adminDir), access: 'read', scope: 'subtree' },
+ ]);
+ } finally {
+ await fs.rm(repo.root, { recursive: true, force: true });
+ }
+ });
+
+ test('refuses a candidate whose refs entry is a plain file', async () => {
+ const fake = await fs.mkdtemp(join(tmpdir(), 'maka-boundary-file-refs-'));
+ await fs.mkdir(join(fake, 'objects'), { recursive: true });
+ await fs.writeFile(join(fake, 'refs'), 'not-a-directory\n');
+ const repo = await worktreeAdminInsideCommonDir(fake);
+ try {
+ const result = await preflightDeclaredSandboxBoundary(
+ {
+ filesystem: {
+ entries: [{ path: repo.adminDir, access: 'read', scope: 'subtree' }],
+ },
+ },
+ toolContext(repo.worktreeDir),
+ );
+
+ assert.deepEqual(result?.filesystem?.entries, [
+ { path: await fs.realpath(repo.adminDir), access: 'read', scope: 'subtree' },
+ ]);
+ assert.ok(!JSON.stringify(result).includes(await fs.realpath(fake)));
Review Comment:
P2: `adminDir` is `fake/worktrees/wt` here, so the unchanged entry always contains `realpath(fake)` as a prefix and this assertion fails (it is the CI failure on this head). The `deepEqual` above already proves the boundary was not widened; drop this line or check that no entry's `path` equals `realpath(fake)`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org