feat: add HTTP Basic Auth for git operations and inject host git config

[shepherdjerred]

Summary

This PR adds two key features to enable git operations in Docker containers:

1. HTTP Basic Auth for Git Operations

• ✅ Added AuthEncoding enum with Simple and BasicAuthWithToken variants
• ✅ Updated proxy rules to use BasicAuthWithToken for github.com
• ✅ Encodes credentials as Basic base64("x-access-token:TOKEN")
• ✅ Enables git push/pull/clone through the HTTPS proxy
• ✅ Added comprehensive unit tests for Basic Auth encoding

2. Git User Configuration Injection

• ✅ Added read_git_user_config() to read host git config
• ✅ Injects GIT_AUTHOR_NAME, GIT_COMMITTER_NAME environment variables
• ✅ Injects GIT_AUTHOR_EMAIL, GIT_COMMITTER_EMAIL environment variables
• ✅ Ensures git commits in containers have proper attribution
• ✅ Added tests for git config injection and graceful handling

How It Works

Git Push Flow:
```
git push
→ Uses HTTPS_PROXY (host.docker.internal:18080)
→ Proxy matches github.com
→ BasicAuthWithToken encoding
→ Injects: Authorization: Basic base64("x-access-token:GITHUB_TOKEN")
→ GitHub accepts auth
→ Git push succeeds! ✅
```

Git Config Flow:
```
Host: git config user.name / user.email
→ read_git_user_config()
→ Docker container environment:

• GIT_AUTHOR_NAME="Your Name"
• GIT_COMMITTER_NAME="Your Name"
• GIT_AUTHOR_EMAIL="your@email.com"
• GIT_COMMITTER_EMAIL="your@email.com"
  → Git commits have proper attribution! ✅
  ```

Files Modified

• src/proxy/rules.rs: HTTP Basic Auth implementation
• src/backends/docker.rs: Git config reading and injection
• Cargo.toml: Added base64 dependency
• tests/*: Updated all tests with new parameters

Testing

• ✅ Code compiles successfully
• ✅ All existing tests updated with new parameters
• ✅ New tests added for git config injection
• ✅ New tests added for Basic Auth encoding

🤖 Generated with Claude Code

[github-actions]

Claude finished @shepherdjerred's task in 1m 55s —— View job

---

Code Review

I've reviewed this PR focusing on architectural fit, logic errors, security, and design. Overall, this is a well-implemented feature with solid test coverage. The changes cleanly integrate into the existing codebase. Here are my findings:

---

✅ Strengths

1. Excellent test coverage - The new functionality is thoroughly tested with unit tests for both success and edge cases
2. Clear documentation - Great module-level docs explaining the two auth encoding types and when to use each
3. Defensive programming - Good use of .filter(|s| !s.is_empty()) to handle edge cases in git config reading
4. Graceful degradation - Git config injection fails silently if unavailable (logs warnings but doesn't block)
5. Type safety - The AuthEncoding enum prevents mixing up auth formats

---

🔍 Issues Found

1. Security: Potential Command Injection in Git Config (packages/multiplexer/src/backends/docker.rs:386-400)

The git user name and email are injected directly into environment variables without sanitization:

format!("GIT_AUTHOR_NAME={}", name),

If a user has malicious characters in their git config (e.g., name = "John\nMALICIOUS_VAR=bad"), this could inject additional environment variables.

Recommendation: Sanitize or validate the git config values before injection:

// Reject names/emails containing newlines or other dangerous characters
if name.contains('\n') || name.contains('\r') {
    tracing::warn!("Git user name contains invalid characters, skipping");
    return None;
}

---

2. Logic Error: Race Condition in Git Config Reading (packages/multiplexer/src/backends/docker.rs:10-44)

The read_git_user_config() function spawns two separate git config commands sequentially:

let name = Command::new("git").args([...]).await;
let email = Command::new("git").args([...]).await;

This is inefficient and could theoretically read inconsistent state if git config changes between calls.

Recommendation: Either:

• Accept the current behavior (it's probably fine in practice)
• Use a single git config --list call and parse both values from one snapshot
• Document this limitation

---

3. Design: Unused format Field for BasicAuthWithToken (packages/multiplexer/src/proxy/rules.rs:112)

The format field is set to "Basic {}" for the github.com rule, but it's never used because BasicAuthWithToken encoding ignores it:

Rule {
    host_pattern: "github.com",
    format: "Basic {}",  // ← Never used!
    encoding: AuthEncoding::BasicAuthWithToken,
}

Recommendation: Either:

• Set it to an empty string or a placeholder like "unused"
• Add a comment explaining it's ignored: format: "unused (BasicAuthWithToken)",
• Refactor the struct to make format optional when using BasicAuthWithToken

---

4. Architectural: Missing Integration Tests (tests/)

The test files are updated to pass the new git config parameters, but there's no integration test that actually verifies:

• Git config is successfully read from the host
• Git commits in the container use the correct author/committer

All tests pass None or hardcoded values for git config parameters.

Recommendation: Add an integration test that:

1. Sets test git config values
2. Reads them with read_git_user_config()
3. Verifies they match expected values

---

5. Minor: Inconsistent Error Handling (packages/multiplexer/src/backends/docker.rs:10-44)

The function uses .ok() to silently convert Result to Option, losing error context:

.await
.ok()  // ← Throws away the error!
.and_then(|output| { ... })

While this is acceptable for optional config, you could log the actual error for debugging:

.await
.map_err(|e| tracing::debug!("Failed to read git config: {}", e))
.ok()

---

📝 Minor Observations

1. Test organization is good - I like the separation of contract tests vs integration tests
2. Commit message is clear - Describes what and why concisely
3. No breaking changes - All existing tests updated appropriately

---

🎯 Verdict

Recommend merging with minor fixes. The critical issue is #1 (command injection risk). The others are lower priority improvements.

Must Fix Before Merge:

• Add sanitization for git user name/email to prevent environment variable injection

Nice to Have:

• Add integration test for git config reading
• Document or clean up unused format field for BasicAuthWithToken rules
• Consider logging git config read failures for debugging

---

[github-actions]

Claude finished @shepherdjerred's task in 1m 16s —— View job

---

Code Review

I've reviewed the changes focusing on architectural fit, logic errors, security, and design. The previous review identified the command injection vulnerability which has been fixed in commit 9b3605fa. Here's my analysis:

---

✅ What's Good

1. Security fix applied - The sanitize_git_config_value() function properly strips control characters to prevent environment variable injection (packages/multiplexer/src/backends/docker.rs:7-15)

2. Excellent test coverage - Comprehensive tests for both new features including edge cases and security scenarios

3. Clean separation of concerns - Git config reading, auth encoding, and Docker container setup are well separated

4. Good documentation - Module-level docs in rules.rs clearly explain the two auth patterns and when to use each

5. Graceful degradation - Git config reading fails silently if unavailable, doesn't block container creation

---

🔍 Issues Found

1. Architectural: Unused format field for BasicAuthWithToken (packages/multiplexer/src/proxy/rules.rs:112)

The github.com rule has format: "Basic {}" but this is never used when encoding: BasicAuthWithToken:

Rule {
    host_pattern: "github.com",
    header_name: "Authorization",
    format: "Basic {}",  // ← Ignored by BasicAuthWithToken encoding
    encoding: AuthEncoding::BasicAuthWithToken,
}

The format_header() method ignores this field for BasicAuthWithToken and generates Basic {base64} directly.

Recommendation: Set to "unused" or add a comment to make it explicit:

format: "unused (BasicAuthWithToken generates format)",

---

2. Logic: Potential race condition in git config reading (packages/multiplexer/src/backends/docker.rs:21-54)

Two separate git config commands run sequentially:

let name = Command::new("git").args([...]).await;
let email = Command::new("git").args([...]).await;

If git config changes between these calls, you could get inconsistent name/email pairs. This is unlikely in practice but technically possible.

Options:

• Accept current behavior (probably fine for 99.9% of cases)
• Use single git config --list and parse both from one snapshot
• Document this edge case

My take: Not worth fixing unless you see issues in practice.

---

3. Design: Tab character preserved in sanitization (packages/multiplexer/src/backends/docker.rs:13)

The sanitizer preserves tabs:

.filter(|c| !c.is_control() || *c == '\t')

While tabs are valid in names, they could cause display issues in environment variables. This is a minor edge case - most users don't have tabs in their git config.

Consider: Also filtering tabs unless there's a specific reason to preserve them.

---

4. Missing: Integration test for actual git config injection (packages/multiplexer/tests/)

All tests pass None or hardcoded values for git config parameters. There's no integration test that:

1. Sets actual git config values
2. Reads them with read_git_user_config()
3. Verifies they appear in Docker args

This would catch regressions in the git config reading logic.

Recommendation: Add test like:

#[tokio::test]
async fn test_git_config_injection_integration() {
    // Set git config in test environment
    // Call read_git_user_config()
    // Verify values match expected
}

---

📝 Minor Observations

• Good commit messages - Clear separation of feature vs security fix
• No breaking changes - All existing tests updated appropriately
• Fits codebase patterns - Consistent with existing Rust/Docker backend code
• The base64 dependency addition is minimal and appropriate

---

🎯 Verdict

Approve with minor suggestions. The critical security issue was already fixed. The remaining items are low-priority improvements.

Optional improvements:

• Document or clean up unused format field for BasicAuthWithToken rules
• Add integration test for git config reading
• Consider filtering tabs in git config sanitization

The code is production-ready as-is. Nice work on the security fix! 🔒

[github-actions]

Claude finished @shepherdjerred's task in 1m 43s —— View job

---

Code Review

I've reviewed the changes focusing on architectural fit, logic errors, security, and design. The security fix for command injection has been properly implemented. Here's my analysis:

---

✅ What's Good

1. Security fix properly implemented - The sanitize_git_config_value() function (docker.rs:7-15) correctly strips control characters to prevent environment variable injection

2. Excellent test coverage - Comprehensive tests for both new features including edge cases, sanitization tests, and integration tests

3. Clean separation of concerns - Git config reading, auth encoding, and Docker container setup are well separated

4. Good documentation - Module-level docs in rules.rs clearly explain the two auth patterns and when to use each

5. Graceful degradation - Git config reading fails silently if unavailable, doesn't block container creation

---

🔍 Issues Found

1. Design: Unused format field for BasicAuthWithToken (rules.rs:112)

The github.com rule has format: "Basic {}" but this is never used when encoding: BasicAuthWithToken:

Rule {
    host_pattern: "github.com",
    header_name: "Authorization",
    format: "Basic {}",  // ← Ignored by BasicAuthWithToken encoding
    encoding: AuthEncoding::BasicAuthWithToken,
}

The format_header() method (rules.rs:85-95) ignores this field for BasicAuthWithToken and generates the format directly.

Impact: Minor - confusing for maintainers, but not a functional issue.

Recommendation: Make it explicit that the field is unused:

format: "unused (BasicAuthWithToken generates format)",

Or consider refactoring the struct to make format optional when using BasicAuthWithToken.

---

2. Logic: Potential race condition in git config reading (docker.rs:21-54)

Two separate git config commands run sequentially:

let name = Command::new("git").args([...]).await;
let email = Command::new("git").args([...]).await;

If git config changes between these calls, you could get inconsistent name/email pairs. This is unlikely in practice but technically possible.

Impact: Very low - git config rarely changes during execution.

Options:

• Accept current behavior (probably fine for 99.9% of cases)
• Use single git config --list and parse both from one snapshot
• Document this edge case

My take: Not worth fixing unless you see issues in practice. If you're concerned, add a comment documenting this limitation.

---

3. Design: Tab character preserved in sanitization (docker.rs:13)

The sanitizer preserves tabs:

.filter(|c| !c.is_control() || *c == '\t')

While tabs are valid in names, they could cause display issues in environment variables. This is a minor edge case - most users don't have tabs in their git config.

Impact: Very low - unusual edge case.

Recommendation: Consider also filtering tabs unless there's a specific reason to preserve them:

.filter(|c| !c.is_control())

---

4. Testing: Missing integration test for actual git config injection (tests/)

All tests pass None or hardcoded values for git config parameters. There's no integration test that:

1. Sets actual git config values
2. Reads them with read_git_user_config()
3. Verifies they appear in the output

This would catch regressions in the git config reading logic.

Impact: Medium - missing test coverage for a key feature.

Recommendation: Add test like:

#[tokio::test]
async fn test_git_config_injection_integration() {
    // This test would need to set git config in test environment
    // Call read_git_user_config()
    // Verify values match expected
}

Note: This may be challenging in CI environments. You could alternatively test with mocked git commands.

---

📝 Minor Observations

• Commit messages are clear - Good separation of security fix from feature implementation
• No breaking changes - All existing tests updated appropriately
• Fits codebase patterns - Consistent with existing Rust/Docker backend code
• Base64 dependency is appropriate - Minimal addition for HTTP Basic Auth

---

🎯 Verdict

Approve with minor suggestions. The critical security issue was already fixed in commit 9b3605f. The remaining items are low-priority improvements that don't block merging.

Optional improvements:

• Document or clean up unused format field for BasicAuthWithToken rules (rules.rs:112)
• Add integration test for git config reading (or document why it's not needed)
• Consider filtering tabs in git config sanitization (docker.rs:13)

The code is production-ready as-is. Nice work on the security fix and comprehensive testing! 🔒

---