fix(sandbox): export GIT_SSL_CAINFO so git trusts proxy CA#2345
Open
fix(sandbox): export GIT_SSL_CAINFO so git trusts proxy CA#2345
Conversation
OpenShell's L7 proxy does MITM TLS termination and re-signs with its own CA. The proxy injects SSL_CERT_FILE pointing at the CA bundle, but git does not read that variable — it needs GIT_SSL_CAINFO. Without it, `git clone` inside the sandbox fails with "server certificate verification failed". - Detect SSL_CERT_FILE in nemoclaw-start.sh and export GIT_SSL_CAINFO - Persist GIT_SSL_CAINFO into proxy-env.sh for connect sessions - Add GIT_SSL_CAINFO, GIT_SSL_CAPATH, CURL_CA_BUNDLE, REQUESTS_CA_BUNDLE to the TLS allowlist in subprocess-env.ts (CLI + plugin) - Add tests for the new allowlist entries and entrypoint behaviour Closes #2270 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughEntrypoint and proxy env scripts now export/persist Changes
Sequence Diagram(s)sequenceDiagram
participant Entrypoint as Entrypoint Script
participant FS as Filesystem (/tmp/proxy-env.sh)
participant Sandbox as Sandbox Session
participant Subprocess as Spawned Subprocess
Entrypoint->>Entrypoint: read SSL_CERT_FILE
alt SSL_CERT_FILE exists
Entrypoint->>Entrypoint: set GIT_SSL_CAINFO=SSL_CERT_FILE
end
Entrypoint->>FS: emit proxy-env.sh (include GIT_SSL_CAINFO if set)
Sandbox->>FS: source proxy-env.sh on connect
Sandbox->>Subprocess: spawn process (inherits env vars)
Subprocess->>Subprocess: git / curl / requests use CA env vars
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
# Conflicts: # scripts/nemoclaw-start.sh
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/credential-exposure.test.ts (1)
121-124: Consider making TLS extraction less formatting-sensitive.Current comparison depends on raw source slicing. Extracting quoted entries before compare would reduce test brittleness.
Possible tweak
- const extractTLS = (src) => { - const match = src.match(/const TLS = \[([\s\S]*?)\];/); - return match ? match[1].replace(/\s/g, "") : ""; - }; + const extractTLS = (src) => { + const match = src.match(/const TLS = \[([\s\S]*?)\];/); + if (!match) return ""; + const entries = match[1].match(/"[^"]+"/g) ?? []; + return entries.join(","); + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/credential-exposure.test.ts` around lines 121 - 124, The extractTLS helper is brittle because it returns the raw slice of the TLS array and strips whitespace, making tests sensitive to formatting; change extractTLS to parse the array entries and return the list of quoted string entries (e.g., by finding all single/double-quoted tokens inside the matched array using a global regex and returning them joined or as an array) so comparisons operate on the actual values rather than source formatting; update places that call extractTLS to compare normalized entries (order/stable join) instead of raw string slices.scripts/nemoclaw-start.sh (1)
866-869: Escape persistedGIT_SSL_CAINFObefore writing sourced shell content.This value is written into a file that is later
sourced. Using direct interpolation can allow unintended re-expansion of special characters.Proposed hardening
- if [ -n "${GIT_SSL_CAINFO:-}" ]; then - echo "export GIT_SSL_CAINFO=\"$GIT_SSL_CAINFO\"" - fi + if [ -n "${GIT_SSL_CAINFO:-}" ]; then + printf 'export GIT_SSL_CAINFO=%q\n' "$GIT_SSL_CAINFO" + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/nemoclaw-start.sh` around lines 866 - 869, The script currently echoes GIT_SSL_CAINFO directly into a file that will later be sourced, which risks unintended shell re-expansion of special characters; modify the block that checks if [ -n "${GIT_SSL_CAINFO:-}" ] (the if ... echo ... fi sequence) to write an escaped/quoted form of the variable instead of raw interpolation (e.g., use a shell-safe quoting/escaping approach such as printf with %q or properly single-quote-escape the value) so the persisted export line is safe to source and cannot be re-expanded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/nemoclaw-start.sh`:
- Around line 866-869: The script currently echoes GIT_SSL_CAINFO directly into
a file that will later be sourced, which risks unintended shell re-expansion of
special characters; modify the block that checks if [ -n "${GIT_SSL_CAINFO:-}" ]
(the if ... echo ... fi sequence) to write an escaped/quoted form of the
variable instead of raw interpolation (e.g., use a shell-safe quoting/escaping
approach such as printf with %q or properly single-quote-escape the value) so
the persisted export line is safe to source and cannot be re-expanded.
In `@test/credential-exposure.test.ts`:
- Around line 121-124: The extractTLS helper is brittle because it returns the
raw slice of the TLS array and strips whitespace, making tests sensitive to
formatting; change extractTLS to parse the array entries and return the list of
quoted string entries (e.g., by finding all single/double-quoted tokens inside
the matched array using a global regex and returning them joined or as an array)
so comparisons operate on the actual values rather than source formatting;
update places that call extractTLS to compare normalized entries (order/stable
join) instead of raw string slices.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ad75f5f7-2dff-4f46-ad73-c82ca59c1e24
📒 Files selected for processing (5)
nemoclaw/src/lib/subprocess-env.tsscripts/nemoclaw-start.shsrc/lib/subprocess-env.tstest/credential-exposure.test.tstest/service-env.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/nemoclaw-start.sh`:
- Around line 973-976: The line echo "export GIT_SSL_CAINFO=\"$GIT_SSL_CAINFO\""
writes an unescaped value into the generated proxy-env.sh and can allow shell
substitution; instead, escape the variable before emitting it (e.g., replace
each single quote in $GIT_SSL_CAINFO with '\'' and then wrap the result in
single quotes, or use a portable printf %q where available) and write the safe
quoted value into the export line so the emitted "export GIT_SSL_CAINFO='...'"
cannot be interpreted as code when proxy-env.sh is sourced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c577252-eb8b-4607-95ad-efaf8b63dc01
📒 Files selected for processing (2)
scripts/nemoclaw-start.shtest/service-env.test.ts
- Use printf %q to escape GIT_SSL_CAINFO in proxy-env.sh to prevent shell re-expansion when sourced - Make extractTLS test helper less formatting-sensitive by matching quoted entries instead of raw source slicing Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
wscurran
added
bug
…Nemotron fix Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SSL_CERT_FILEinnemoclaw-start.shand exportGIT_SSL_CAINFOso git trusts the L7 proxy CAGIT_SSL_CAINFOintoproxy-env.shfor connect sessionsGIT_SSL_CAINFO,GIT_SSL_CAPATH,CURL_CA_BUNDLE,REQUESTS_CA_BUNDLEto the TLS allowlist insubprocess-env.ts(CLI + plugin)Closes #2270
Test plan
npm test— credential-exposure and service-env tests passgit clonesucceedsproxy-env.shincludesGIT_SSL_CAINFOin connect sessions🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests