Skip to content

fix(cli): fix secure_getenv() bypass of one-shot token protection#1244

Open
Mossaka wants to merge 2 commits intomainfrom
fix/055-secure-getenv-bypass
Open

fix(cli): fix secure_getenv() bypass of one-shot token protection#1244
Mossaka wants to merge 2 commits intomainfrom
fix/055-secure-getenv-bypass

Conversation

@Mossaka
Copy link
Collaborator

@Mossaka Mossaka commented Mar 11, 2026

Summary

  • Fix critical bug where secure_getenv() in the C one-shot-token library bypassed all token protection when called before getenv()
  • secure_getenv() was calling get_token_index() before init_token_list() and without the mutex, so the empty token list always returned -1 (not sensitive), passing raw token values through
  • Added initialization, mutex acquisition, and recursion guard to secure_getenv() matching the correct getenv() implementation

Fixes #756

Test plan

  • npm run build passes
  • npm test passes (856 tests)
  • npm run lint passes
  • Rust implementation already correct (uses shared handle_getenv_impl)
  • CI integration tests pass (one-shot-tokens.test.ts)

🤖 Generated with Claude Code

Copilot AI review requested due to automatic review settings March 11, 2026 18:46
@github-actions
Copy link
Contributor

github-actions bot commented Mar 11, 2026 •
edited
Loading

✅ Coverage Check Passed

Overall Coverage

Metric Base PR Delta
Lines 84.23% 84.36% 📈 +0.13%
Statements 84.21% 84.34% 📈 +0.13%
Functions 84.37% 84.37% ➡️ +0.00%
Branches 77.09% 77.17% 📈 +0.08%
📁 Per-file Coverage Changes (1 files)
File Lines (Before → After) Statements (Before → After)
src/docker-manager.ts 86.8% → 87.3% (+0.52%) 86.1% → 86.6% (+0.50%)

Coverage comparison generated by scripts/ci/compare-coverage.ts

Copilot AI reviewed Mar 11, 2026
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a critical security flaw in the agent’s one-shot-token LD_PRELOAD library where secure_getenv() could bypass token protection if invoked before getenv(), ensuring sensitive environment variables are still cached/unset on first access.

Changes:

  • Add mutex acquisition and init_token_list() initialization to secure_getenv() before calling get_token_index().
  • Add the same thread-local recursion guard behavior to secure_getenv() that getenv() already uses, preventing re-entrant deadlocks.
  • Ensure the non-sensitive path releases the mutex and clears the recursion guard before returning.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 11, 2026

Smoke Test Results

Recent merged PRs:

Test Result
GitHub MCP (list merged PRs) ✅
Playwright (github.com title) ✅
File write ✅
Bash verify ✅

Overall: PASS

💥 [THE END] — Illustrated by Smoke Claude for issue #1244

@github-actions
Copy link
Contributor

github-actions bot commented Mar 11, 2026

Smoke Test Results

✅ GitHub MCP — Last 2 merged PRs:

✅ Playwright — github.com title contains "GitHub"
✅ File Write — /tmp/gh-aw/agent/smoke-test-copilot-22968955590.txt created and verified
✅ Bash — File read back successfully

Overall: PASS

📰 BREAKING: Report filed by Smoke Copilot for issue #1244

@github-actions
Copy link
Contributor

github-actions bot commented Mar 11, 2026

PR titles:
test: expand credential hiding tests to all 14 protected paths
test(docker): verify capsh execution chain after PR #715
Tests: MCP ✅ | safeinputs-gh ✅ | Playwright ✅ | Tavily ❌ | File+cat ✅ | Discussion ✅ | Build ✅
Overall: FAIL

🔮 The oracle has spoken through Smoke Codex for issue #1244

@github-actions github-actions bot mentioned this pull request Mar 11, 2026
Closed
@github-actions
Copy link
Contributor

github-actions bot commented Mar 11, 2026

Chroot Version Comparison Results

Runtime Host Version Chroot Version Match?
Python Python 3.12.12 Python 3.12.3 ❌ NO
Node.js v24.14.0 v20.20.0 ❌ NO
Go go1.22.12 go1.22.12 ✅ YES

Overall: ❌ FAILED — Python and Node.js versions differ between host and chroot environments.

Tested by Smoke Chroot for issue #1244

@Mossaka @claude
fix(cli): fix secure_getenv() bypass of one-shot token protection
7cde54a 
secure_getenv() was calling get_token_index() before init_token_list()
and without the mutex, causing all token protection to be bypassed when
secure_getenv() was the first call into the library (empty token list
returns -1 for all lookups). Added initialization, mutex acquisition,
and recursion guard matching the getenv() implementation.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@Mossaka Mossaka force-pushed the fix/055-secure-getenv-bypass branch from d780697 to 7cde54a Compare March 12, 2026 23:20
@github-actions
Copy link
Contributor

github-actions bot commented Mar 12, 2026

Smoke Test Results — claude-sonnet-4-6

Test Result
GitHub MCP: feat(proxy): add GitHub Enterprise Cloud/Server support (#1264) ✅
GitHub MCP: fix(cli): fix secure_getenv() bypass (#1244 current) ✅
Playwright: github.com title contains "GitHub" ✅
File write + read back ✅

Overall: PASS

💥 [THE END] — Illustrated by Smoke Claude for issue #1244

@github-actions
Copy link
Contributor

github-actions bot commented Mar 12, 2026

PR Titles:
feat(proxy): add GitHub Enterprise Cloud/Server support with automatic endpoint detection
fix: drop -f from curl to avoid GitHub API rate-limit flakiness
Test 1 GitHub MCP review: ✅
Test 2 safeinputs-gh: ✅
Test 3 Playwright: ✅
Test 4 Tavily search: ❌
Test 5-7 file/bash/discussion: ✅
Test 8 build: ✅
Overall: FAIL

🔮 The oracle has spoken through Smoke Codex for issue #1244

@github-actions
Copy link
Contributor

github-actions bot commented Mar 12, 2026

Chroot Version Comparison Results

Runtime Host Version Chroot Version Match?
Python Python 3.12.13 Python 3.12.3 ❌ NO
Node.js v24.14.0 v20.20.1 ❌ NO
Go go1.22.12 go1.22.12 ✅ YES

Overall: ❌ FAILED — Python and Node.js versions differ between host and chroot environments.

Tested by Smoke Chroot for issue #1244

@github-actions
Copy link
Contributor

github-actions bot commented Mar 12, 2026

Smoke Test Results — Run 23028546935

Test Result
GitHub MCP (last 2 merged PRs) ✅ #1267 fix: drop -f from curl to avoid GitHub API rate-limit flakiness, #1265 fix: add missing formatItem and program imports in cli.test.ts
Playwright (github.com title) ✅ Title contains "GitHub"
File write (smoke-test-copilot-23028546935.txt) ✅
Bash verify (cat) ✅

Overall: PASS

PR author: @Mossaka · No assignees.

📰 BREAKING: Report filed by Smoke Copilot for issue #1244

@github-actions
Copy link
Contributor

github-actions bot commented Mar 12, 2026

🏗️ Build Test Suite Results

Ecosystem Project Build/Install Tests Status
Bun elysia ✅ 1/1 passed ✅ PASS
Bun hono ✅ 1/1 passed ✅ PASS
C++ fmt ✅ N/A ✅ PASS
C++ json ✅ N/A ✅ PASS
Deno oak N/A 1/1 passed ✅ PASS
Deno std N/A 1/1 passed ✅ PASS
.NET hello-world ✅ N/A ✅ PASS
.NET json-parse ✅ N/A ✅ PASS
Go color ✅ 1/1 passed ✅ PASS
Go env ✅ 1/1 passed ✅ PASS
Go uuid ✅ 1/1 passed ✅ PASS
Java gson ✅ 1/1 passed ✅ PASS
Java caffeine ✅ 1/1 passed ✅ PASS
Node.js clsx ✅ All passed ✅ PASS
Node.js execa ✅ All passed ✅ PASS
Node.js p-limit ✅ All passed ✅ PASS
Rust fd ✅ 1/1 passed ✅ PASS
Rust zoxide ✅ 1/1 passed ✅ PASS

Overall: 8/8 ecosystems passed — ✅ PASS

Notes
  • Java: Maven proxy configured with Squid IP (172.30.0.10:3128) instead of hostname squid-proxy (not DNS-resolvable); JAVA_TOOL_OPTIONS cleared to avoid hostname-based proxy conflict.
  • Rust: LD_PRELOAD (one-shot-token.so) causes rustc/cargo to hang; cleared for Rust builds.

Generated by Build Test Suite for issue #1244 · ◷

@Mossaka Mossaka enabled auto-merge (squash) March 13, 2026 00:11
@Mossaka @claude
fix: move token init to constructor to prevent contention
6b8c568 
Multi-threaded programs like rustc call getenv() from many threads during
startup. The previous fix held the mutex for every getenv() call to protect
lazy initialization, which serialized all threads and caused rustc to
timeout (60s) in CI.

Move init_token_list() into the library constructor which runs before any
threads are created. This eliminates the data race without needing the
mutex for non-sensitive variable lookups.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions bot mentioned this pull request Mar 13, 2026
Open
@github-actions
Copy link
Contributor

github-actions bot commented Mar 13, 2026

Smoke Test Results

Test Status
GitHub MCP (last 2 merged PRs: #1264, #1245) ✅
Playwright (github.com title contains "GitHub") ✅
File write (smoke-test-claude-23030235945.txt) ✅
Bash verify (file content confirmed) ✅

Overall: PASS

💥 [THE END] — Illustrated by Smoke Claude for issue #1244

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

Assignees

No one assigned

Labels

build-test smoke-claude smoke-copilot

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Security] secure_getenv() bypasses one-shot token protection when called before getenv()

2 participants

@Mossaka