harden agent workspace boundaries

[CaeruleusAqua]

What was done and why?

• Centralizes workspace path validation and rejects absolute paths, traversal, Git metadata, and symlink escapes.
• Prevents repository tools from recursively reading Git metadata or files reached through symlinks.
• Keeps Git credentials outside cloned repositories in a private temporary parent with owner-only permissions and cleanup.
• Runs trusted host-side Git operations with empty hooks, scrubbed environments, and isolated global configuration.
• Supports an optional host-visible root while retaining private workspace parents for Docker sandbox mounts.
• Adds regressions for case-insensitive Git metadata, path normalization, symlinks, credential placement, cleanup, configured workspace roots, hooks, and fsmonitor.

Testing

• Build and verify CI
• Focused workspace-boundary regression tests
• Git whitespace validation

AI use

• Initial implementation used KIMI K3; the split, hardening, and review used OpenCode (gpt-5.6-terra).

[max-california]

🤖 Agentic Code Review

Summary: the hardening looks solid overall. Centralizing path validation, blocking .git access during direct and recursive reads, and moving credentials outside the repo all head in the right direction. I did find one correctness issue in the clone fallback flow that can leave stale credential files behind.

BLOCKER

1. Credential file leak on branch-clone fallback
   • File: src/main/java/org/remus/giteabot/agent/validation/WorkspaceService.java
   • Lines: around 83–86
   • In prepareWorkspace, when the initial git clone --branch ... fails and prNumber != null, you call cleanupWorkspace(workspaceDir) and then immediately reassign workspaceDir = createWorkspaceDirectory(); credentialsFile = createCredentialsFile(...).
   • The problem is that the first workspace root is deleted before the old credentialsFile reference is cleared, and then credentialsFile is overwritten with the second file path. If cleanupWorkspace(workspaceDir) fails partially (which it already tolerates by only logging), the first credential store file can be left behind with no remaining reference for deleteCredentialsFile(...) to clean up later.
   • Because this code is specifically handling access tokens, I’d treat that as a merge-blocking cleanup bug.
   • Suggested fix: either:
     • explicitly deleteCredentialsFile(credentialsFile) before overwriting it in the fallback path, and then null it out, or
     • keep the workspace root/credential lifecycle together in a small holder object so the old credential file cannot be dropped during retry paths.

LOW

1. persistCredentialHelper ignores failures while making later authenticated operations depend on it

   • File: src/main/java/org/remus/giteabot/agent/validation/WorkspaceService.java
   • Lines: around 430–438
   • Both git config --local ... credential.helper commands discard their results. If either fails, later fetch/push operations may fail in less obvious ways.
   • Not necessarily a blocker because clone already succeeded and cleanup still works, but logging or checking these results would make failures much easier to diagnose.

2. Path guard now rejects benign normalized paths like src/../inside.txt

   • File: src/main/java/org/remus/giteabot/util/WorkspacePaths.java
   • Lines: around 31–40
   • This is a deliberate hardening choice, but it is stricter than the previous implementation and may reject user/tool input that still normalizes safely inside the workspace.
   • If that compatibility change is intentional, consider documenting it where tool inputs are described; otherwise callers may see surprising “traversal” errors for paths that are semantically in-bounds.

---

Read-only agentic review by AI Git Bot

[CaeruleusAqua]

Addressed in 79665df ("fix(security): bind credential lifecycle to workspace setup").

BLOCKER — credential file leak on branch-clone fallback: Fixed. The workspace root and its credential-store file now live in a single WorkspaceSetup holder (WorkspaceSetup.java), which is the unit of cleanup: prepareWorkspace tears down the failed attempt — parent directory and credential file — via cleanupWorkspace(WorkspaceSetup) before starting the retry, and the retry creates its own holder. The old defunct pattern (an unregistered Path that was silently overwritten) no longer exists; the catch/error paths clean up the holder instead of two detached references. Regression tests: prepareWorkspace_fallbackRetainsExactlyOneWorkspaceDirectory (no orphaned first attempt remains after the fallback) and cleanupWorkspace_setupDeletesCredentialFileAndRootTogether (the holder removes the never-registered credential file together with the private parent).

LOW 1 — persistent credential.helper: Already addressed by the current code: credentials are passed as per-command git -c arguments (withCredentialConfig/credentialConfigArgs) instead of a persisted git config --local credential.helper, so no configuration write can fail silently; failures surface directly in the command result.

LOW 2 — strict .. rejection: Kept by design, now documented in the WorkspacePaths class Javadoc: any .. segment is rejected up front even if normalization would stay in-bounds; callers must pass resolved or plain relative paths.

[tmseidel]

Just the two small code-smells, the rest looks good to me 👍

[tmseidel]

this must be 6 not 7, since command[0] will be already inserted

Suggested change

	List<String> gitCommand = new ArrayList<>(command.length + 7);
	List<String> gitCommand = new ArrayList<>(command.length + 6);

[tmseidel]

The for-loop can be written more easily with

gitCommand.addAll(Arrays.asList(command).subList(1, command.length));

[CaeruleusAqua]

Both code smells are fixed in 729f1bb:

1. new ArrayList<>(command.length + 7) → command.length + 6 — correct, the list holds command[0] + 6 fixed -c arguments + the remaining command.length - 1 arguments, i.e. exactly command.length + 6 entries.
2. Manual copy loop → gitCommand.addAll(Arrays.asList(command).subList(1, command.length)) — done, with the java.util.Arrays import added.

WorkspaceServiceTest + WorkspacePathsTest are green (20 tests), git diff --check clean. Regarding the two AI-review LOWs: persistCredentialHelper no longer exists (credentials are passed per-command via git -c credential.helper=store --file=…, so failures surface directly), and the strict .. rejection in WorkspacePaths is intentional and now documented in its Javadoc.