feat(agents): Run as moatuser with --dangerously-skip-permissions as default

[dpup]

This PR addresses code review feedback from PRs #35 and #38, adds several enhancements for better UX and
security, and implements non-root container execution to enable --dangerously-skip-permissions by default.

Non-Root Container Execution

Containers now run as moatuser (UID 1000) instead of root, enabling Claude Code's
--dangerously-skip-permissions flag which was previously blocked for security reasons when running as root.

• Creates moatuser in generated Dockerfiles with proper home directory structure
• Installs gosu for privilege dropping in entrypoint
• Dynamic UID mapping on Linux: Detects host UID and passes --user UID:GID to ensure workspace file
  permissions work correctly regardless of host user
• On macOS/Windows, Docker Desktop handles UID translation automatically

--dangerously-skip-permissions by Default

Since Moat runs Claude Code in an isolated container, the container itself serves as the security boundary.
Permission prompts are now skipped by default, with --noyolo available to restore them.

Security Fixes

• Command injection prevention: Fixed potential command injection in runMarketplacesUpdate() using validated
  path functions
• Input validation: Added POSIX-compliant environment variable name validation, symlink resolution for
  workspace paths, and host validation in proxy credential methods
• Duplicate mount fix: Removed duplicated provider mounts code that caused "Duplicate mount point" errors

Error Handling Improvements

• Created cleanupProxy(), cleanupSSH(), and cleanupClaude() helper functions that log errors instead of
  silently discarding them
• Fixed claudeGenerated temp directory cleanup lifecycle by tracking in Run struct
• Added debug logging for credential detection, cleanup errors, and session state sync failures

Documentation & Code Quality

• Added package-level documentation (internal/claude/doc.go)
• Documented ~/.claude.json mount security model
• Added security tests for path traversal and URL parsing edge cases
• Added moatuser to valid username test cases
• Fixed gofmt formatting issues

Other

• Added agent.yaml for developing moat with moat claude

[claude]

📦 Previous Review (superseded by newer review)

Pull Request Review: Non-Root Container Execution and Security Improvements

This is a comprehensive PR that addresses multiple security concerns and improves the UX for running Claude Code in containers. The changes are well-structured and the commit history shows thoughtful iteration based on feedback.

Code Quality: Excellent

Strengths:

• Clear commit messages following conventional commits format
• Comprehensive test coverage for security-sensitive functions
• Good separation of concerns with helper functions
• Excellent documentation in internal/claude/doc.go
• Proper error handling with debug logging instead of silent failures

Areas for consideration:

1. UID 1000 hardcoding: While UID 1000 is standard, some base images may have conflicts
2. IPv6 literal parsing: The parsing looks correct but lacks validation for malformed brackets
3. Environment variable regex: Consider adding a comment explaining the POSIX standard

Potential Bugs: Minor Issues

1. SSH socket verification timing: The socat validation only checks if socat is running and socket exists
2. Host UID detection: Does not handle sudo scenario where os.Getuid() returns 0

Performance: Good

• Cleanup helpers prevent resource leaks
• Regex compilation at package init time is efficient
• Proper use of context for cancellation

Security: Strong

Major improvements:

1. Command injection fix: Using MarketplacePath() with validation
2. Input validation: POSIX-compliant env var name validation
3. Path traversal prevention: Comprehensive checks for control characters
4. SSH socket permissions: Changed from 0777 to 0660 with proper ownership
5. Host validation: Defense-in-depth validation in credential injection
6. Symlink resolution: Prevents symlink-based attacks on workspace paths

Concerns:

1. SSH socket timing: Script continues execution even if SSH setup fails
2. Marketplace name validation: Allows filenames like three dots

Test Coverage: Excellent

New tests added:

• Path traversal tests
• Control character validation
• IPv6 literal parsing
• SSH URL edge cases

Suggestions:

1. Add test for hasDependency() with edge cases
2. Add test for UID mapping logic on Linux vs macOS/Windows
3. Add test for validEnvKey regex patterns

Recommendations

Must fix before merge:

• None - the code is production-ready

Should consider:

1. Handle sudo scenario for UID detection on Linux
2. Make SSH socket setup failures fatal when SSH is required
3. Add tests for hasDependency() and validEnvKey regex

Overall Assessment: Approve

This is high-quality work that significantly improves both security and UX. The code demonstrates thoughtful security design, excellent test coverage, clear documentation, and proper error handling.

The issues identified are minor and mostly edge cases. The PR is ready to merge with optional follow-up improvements.

Impact Summary:

• Security: Multiple vulnerability fixes
• UX: Smoother Claude Code experience with --dangerously-skip-permissions by default
• Code quality: Better error handling and cleanup
• Documentation: Comprehensive package docs

Great work addressing the feedback from PRs 35 and 38!

[claude]

Pull Request Review: Non-Root Container Execution and Security Improvements

This is a well-engineered PR that addresses important security concerns and improves UX. The implementation quality is high with excellent defense-in-depth security practices. However, there are a few critical issues to address.

Overall Assessment

Strengths: Clean code, comprehensive path traversal protection, effective code deduplication, strong security test coverage

Concerns: Unsafe fallback to root execution, missing test coverage for UID mapping, security implications of --dangerously-skip-permissions default

Critical Issues

1. Unsafe Fallback to Root Execution (internal/deps/scripts/moat-init.sh:103-112)

If container starts as root but moatuser doesn't exist, script silently executes as root, defeating the security model.

Fix: Fail with clear error instead of falling back to root.

2. UID 1000 Collision (internal/deps/dockerfile.go:47-50)

Deleting existing UID 1000 user doesn't change file ownership. New moatuser inherits access to those files.

Fix: Use higher UID (5000+), add validation, or document clearly.

High Priority

3. Missing UID Mapping Tests

Linux UID mapping with UID 1000 special-casing lacks tests. Critical for security boundaries.

4. Token Extraction Fragility (cmd/moat/cli/grant.go:404-415)

Hardcoded prefix without format validation. Add regex validation.

5. --dangerously-skip-permissions Default

Changes user expectations. Consider: warning on first run, opt-in initially, audit logging, or clearer flag name.

Positive Highlights

Excellent Command Injection Prevention (cmd/moat/cli/claude.go:24-47): Defense-in-depth with centralized validation and comprehensive tests.

Clean Refactoring: Cleanup helpers eliminate duplication effectively.

Strong Documentation (internal/claude/doc.go): Clear package docs improve contributor experience.

Comprehensive Security Tests: Path traversal tests cover all edge cases.

Summary

Significant security improvement. High code quality and good documentation.

Required: Fix root execution fallback (#1) and consider UID collision (#2)

Recommended: Add UID mapping tests, improve token validation, reconsider skip-permissions default

Recommendation: Request changes for #1-2. Ready to merge once addressed.

Great work!

[claude]

Code Review: Non-Root Container Execution & Security Improvements

This is a substantial and well-executed PR that addresses multiple security concerns, improves error handling, and enables --dangerously-skip-permissions by default through non-root container execution. Overall, the code quality is high and the changes are well-tested.

---

✅ Strengths

Security Improvements

1. Non-root container execution - The shift from root to moatuser (UID 5000) is well-implemented with smart UID selection avoiding collisions
2. Excellent input validation with defense-in-depth approach
3. Command injection prevention properly addressed
4. SSH socket permissions reduced from 0777 to 0660

Code Quality

1. Comprehensive test coverage for OAuth token extraction, UID mapping, and security edge cases
2. Well-structured error handling with dedicated cleanup functions
3. Good documentation in moat-init.sh and package docs

---

🔍 Issues & Suggestions

High Priority

1. Potential issue in UID mapping (manager.go:1035-1045)
The code assumes the process UID/GID are the workspace owner UID/GID. This may not be true if moat is run with sudo or as a different user.

Suggestion: Use os.Stat(opts.Workspace) to get the actual workspace owner UID/GID instead of the process UID/GID.

2. SSH socket validation timing (moat-init.sh:35-40)
The validation happens immediately after 0.5s loop. If socat takes longer, this could produce false warnings.

3. User deletion may fail silently (dockerfile.go:131-132)
The || true suppresses all errors, including failures that might matter.

Medium Priority

4. Hardcoded UID 5000 in multiple places - consider shared constant
5. OAuth token regex {20,} minimum may be too restrictive
6. Missing hasDependency() fix mentioned in commit message

Low Priority

7. Test coverage gaps for E2E non-root execution flow
8. Documentation mentions --noyolo flag but implementation not visible in diff

---

🔒 Security Assessment

Strengths:

• Command injection, input validation, non-root execution all excellent
• SSH socket permissions properly tightened
• Proper cleanup of sensitive files

Concerns:

• UID mapping assumption (High)
• Silent user deletion errors (Medium)

---

📝 Recommendations

Before Merge

1. Fix UID mapping to use workspace ownership (High)
2. Verify hasDependency() fix is included (Medium)

Post-Merge

1. Add E2E tests for non-root container execution
2. Consider making moatuser UID configurable

---

✨ Conclusion

High-quality PR that significantly improves security posture. The main concern is the UID mapping assumption which could cause permission issues. Once addressed, ready to merge.

Recommendation: Approve with minor changes requested.

Great work! 🚀

[claude]

Security Review: Non-Root Container Execution and Security Improvements

Executive Summary

This PR introduces significant security enhancements including non-root container execution, input validation improvements, path traversal prevention, and better error handling. The changes are well-implemented with good test coverage. I have identified a few areas for improvement, but no critical security vulnerabilities.

---

Security Issues

MEDIUM Priority

1. Potential Root Execution When Workspace is Root-Owned

Location: internal/run/manager.go:1054-1066

Issue: The UID mapping logic uses the workspace owner UID, but does not prevent running as UID 0 if the workspace is owned by root.

Recommendation: Add validation to prevent running as root when workspaceUID == 0

2. UID 5000 Collision Risk in Non-Standard Environments

Location: internal/deps/dockerfile.go:49

Issue: While UID 5000 is chosen to avoid conflicts with typical user ranges, some enterprise systems use UIDs starting at 5000.

Recommendation: Consider using a higher UID range (e.g., 10000+) or document the UID 5000 choice in user-facing docs for enterprise environments.

---

LOW Priority

• Environment Variable Validation: Could blocklist dangerous variables like LD_PRELOAD
• Host Validation: May reject valid IPv6 addresses
• OAuth Token Regex: Could be more restrictive if token length is known

---

Security Best Practices - Excellent Implementations

1. Path Traversal Prevention

Location: internal/claude/marketplace.go:41-63

Excellent defense-in-depth implementation with 13 comprehensive test cases covering edge cases!

2. Command Injection Prevention

Location: cmd/moat/cli/claude.go:321-332

Well-designed fix with multiple validation layers using MarketplacePath() and filepath.Clean comparison.

3. Constant-Time Authentication

Location: internal/proxy/proxy.go:349

Uses subtle.ConstantTimeCompare to prevent timing attacks - good security practice!

4. Privilege-Dropping Script Security

Location: internal/deps/scripts/moat-init.sh

Well-designed mechanism using gosu with proper validation and safe failure handling.

---

Test Coverage Assessment

Strong Coverage:

• Path traversal prevention (13 test cases)
• Host pattern matching (30+ test cases)
• OAuth token extraction (5 test cases)
• Security edge cases (spoofed domains, control characters)

Missing Coverage:

• Environment variable name validation
• UID mapping logic
• Cleanup function error handling

Recommendation: Add unit tests for env var validation and UID mapping edge cases.

---

Overall Assessment

Grade: A-

This is a well-executed security enhancement with strong attention to detail. The PR significantly improves security posture by:

1. Enabling non-root container execution
2. Preventing path traversal attacks
3. Fixing command injection vulnerabilities
4. Improving resource cleanup and error handling
5. Adding comprehensive input validation

Identified issues are minor and mostly represent opportunities for further hardening rather than actual vulnerabilities.

---

Recommendations Summary

Should Address:

1. MEDIUM: Add check to prevent container running as UID 0 when workspace is root-owned
2. MEDIUM: Document UID 5000 choice in user-facing docs for enterprise environments

Nice to Have:

1. Consider blocklist for dangerous environment variables
2. Improve IPv6 address support in isValidHost
3. Extract cleanup helpers to reusable functions
4. Add unit tests for env var validation and UID mapping

---

Recommendation: Approve with minor suggestions for follow-up improvements. The security enhancements are sound and the code quality is high. Great work on the comprehensive test coverage and security-first approach!