From 4950a4c54e7c48d33bdfe65fd03cb8370ab31454 Mon Sep 17 00:00:00 2001 From: Raul Date: Fri, 13 Mar 2026 22:22:10 +0100 Subject: [PATCH 1/4] Reapply "Add GitHub PAT setup and devcontainer auth (#4)" (#5) This reverts commit 89ad9921319f10c4d7ba8018b94ba605453a508a. --- .devcontainer/devcontainer.json | 1 + .devcontainer/post_install.py | 86 ++++++++++++ README.md | 144 ++++++++++++++++--- personas/developer/CLAUDE.md | 10 ++ personas/web3-auditor/CLAUDE.md | 242 ++++++++++++++++++++++++++++++-- scripts/setup-gh-token.sh | 162 +++++++++++++++++++++ 6 files changed, 620 insertions(+), 25 deletions(-) create mode 100755 scripts/setup-gh-token.sh diff --git a/.devcontainer/devcontainer.json b/.devcontainer/devcontainer.json index 381d0f2..deaf8dd 100644 --- a/.devcontainer/devcontainer.json +++ b/.devcontainer/devcontainer.json @@ -65,6 +65,7 @@ "PIP_DISABLE_PIP_VERSION_CHECK": "1", "GOPATH": "/home/vscode/go", "GOBIN": "/home/vscode/go/bin", + "GH_TOKEN": "${localEnv:GH_TOKEN}", "PERSONA": "developer" }, "initializeCommand": "test -f \"$HOME/.gitconfig\" || touch \"$HOME/.gitconfig\"", diff --git a/.devcontainer/post_install.py b/.devcontainer/post_install.py index 38a92bb..b658ca3 100644 --- a/.devcontainer/post_install.py +++ b/.devcontainer/post_install.py @@ -386,6 +386,91 @@ def setup_global_gitignore(): ) +def _warn_broad_token(token: str, source: str): + """Warn if token is not a fine-grained PAT (github_pat_*).""" + if token.startswith("github_pat_"): + return + if token.startswith("ghp_"): + kind = "classic PAT (ghp_*)" + elif token.startswith("gho_"): + kind = "OAuth token (gh auth login)" + else: + kind = "non-fine-grained token" + print( + f"[post_install] Warning: {source} is a {kind}. " + f"Fine-grained PATs (github_pat_*) are recommended — " + f"they scope access to specific repos with limited " + f"permissions. Run 'bash scripts/setup-gh-token.sh' " + f"on the host to set one up.", + file=sys.stderr, + ) + + +def setup_gh_auth(): + """Configure GitHub auth for gh CLI and git HTTPS credential helper. + + Uses GH_TOKEN env var if set, otherwise checks gh auth status + from the persistent volume. Runs gh auth setup-git to configure + the git credential helper so both gh API calls and git push/pull + work with one token. Never blocks container creation. + """ + gh_token = os.environ.get("GH_TOKEN", "") + + if gh_token: + result = subprocess.run( + ["gh", "api", "user", "--jq", ".login"], + capture_output=True, + text=True, + env={**os.environ, "GH_TOKEN": gh_token}, + ) + if result.returncode == 0 and result.stdout.strip(): + login = result.stdout.strip() + print( + f"[post_install] GitHub auth via GH_TOKEN: {login}", + file=sys.stderr, + ) + _warn_broad_token(gh_token, "GH_TOKEN") + else: + print( + "[post_install] Warning: GH_TOKEN is set but " + "invalid or expired", + file=sys.stderr, + ) + else: + result = subprocess.run( + ["gh", "auth", "status"], + capture_output=True, + ) + if result.returncode != 0: + print( + "[post_install] Warning: No GitHub auth found. " + "Set GH_TOKEN on the host before building, or " + "run 'gh auth login' inside the container.", + file=sys.stderr, + ) + return + # Check token type from gh auth + token_result = subprocess.run( + ["gh", "auth", "token"], + capture_output=True, + text=True, + ) + if token_result.returncode == 0 and token_result.stdout.strip(): + _warn_broad_token( + token_result.stdout.strip(), "gh auth token" + ) + + subprocess.run( + ["gh", "auth", "setup-git"], + capture_output=True, + ) + print( + "[post_install] Git credential helper configured " + "(gh auth setup-git)", + file=sys.stderr, + ) + + def main(): """Run all post-install configuration.""" print( @@ -398,6 +483,7 @@ def main(): apply_local_overlay() setup_tmux_config() setup_global_gitignore() + setup_gh_auth() print("[post_install] Configuration complete!", file=sys.stderr) diff --git a/README.md b/README.md index ab22cba..e07762e 100644 --- a/README.md +++ b/README.md @@ -27,6 +27,7 @@ Then inside the session, run `/trailofbits:config`. It walks you through install **[Configuration](#configuration)** - [Sandboxing](#sandboxing) +- [GitHub Authentication](#github-authentication) - [Hooks](#hooks) - [Plugins and Skills](#plugins-and-skills) - [MCP Servers](#mcp-servers) @@ -141,6 +142,42 @@ claude-local() { `claude-local` wraps `claude` with the local server env vars and disables telemetry pings that won't reach Anthropic anyway. Use it anywhere you'd normally run `claude`. +If you're using the [devcontainer](#devcontainer), also add: + +```bash +export CLAUDE_CODE_CONFIG_DIR="$HOME/Repos/claude-code-config" # adjust path + +claude-container() { + local persona="${1:-developer}" + local project_dir="$PWD" + local project_name + project_name=$(basename "$project_dir") + + devcontainer up \ + --workspace-folder "$CLAUDE_CODE_CONFIG_DIR" \ + --mount "source=${project_dir},target=/workspace/${project_name},type=bind" \ + --remote-env "PERSONA=${persona}" + + devcontainer exec \ + --workspace-folder "$CLAUDE_CODE_CONFIG_DIR" \ + sh -c "cd '/workspace/${project_name}' && exec claude" +} +``` + +`claude-container` builds the devcontainer from this repo, bind-mounts your current directory into `/workspace/`, and launches Claude inside it. Pass a persona name as the first argument to override the default (`developer`): + +```bash +cd ~/Repos/my-project +claude-container # developer persona +claude-container l1-auditor # L1 auditor persona +``` + +The mount is set at container creation time. To switch projects, stop the container and re-run from the new directory, or add `--remove-existing-container` to force a rebuild: + +```bash +devcontainer up --workspace-folder "$CLAUDE_CODE_CONFIG_DIR" --remove-existing-container +``` + ### Settings Copy `personas/developer/settings.json` to `~/.claude/settings.json` (or merge entries into your existing file). The `$schema` key enables autocomplete and validation in editors that support JSON Schema. The template includes: @@ -193,7 +230,7 @@ Review and customize it for your own preferences. The template is opinionated -- |---------|-----|--------| | **developer** | General software development, code review, feature work | Complete | | **l1-auditor** | L1 blockchain security: Cosmos SDK, Geth, consensus-execution coupling | Complete | -| **web3-auditor** | Smart contract auditing, blockchain security | Placeholder | +| **web3-auditor** | Smart contract auditing, blockchain security | Complete | | **pentesting** | Penetration testing, offensive security | Placeholder | Install a persona by copying its directory contents: @@ -257,26 +294,24 @@ Then rebuild the container. The persona's settings.json is installed with `bypas npm install -g @devcontainers/cli ``` -**Option A: CLI (build from this repo, mount any project)** +**Option A: CLI (recommended)** -```bash -# Build and start the container from this repo -devcontainer up --workspace-folder /path/to/claude-code-config +Use the `claude-container` shell function from [Shell Setup](#shell-setup): -# Open a shell inside it -devcontainer exec --workspace-folder /path/to/claude-code-config zsh - -# Run Claude Code inside the container -devcontainer exec --workspace-folder /path/to/claude-code-config claude +```bash +cd ~/Repos/my-project +claude-container # developer persona +claude-container l1-auditor # override persona ``` -To work on a different project, add a bind mount to `devcontainer.json`: +Or run the commands manually: -```jsonc -"mounts": [ - // ... existing mounts ... - "source=/path/to/your-project,target=/workspace/your-project,type=bind" -] +```bash +devcontainer up --workspace-folder /path/to/claude-code-config \ + --mount "source=$PWD,target=/workspace/my-project,type=bind" + +devcontainer exec --workspace-folder /path/to/claude-code-config \ + sh -c "cd /workspace/my-project && claude" ``` **Option B: VS Code / Cursor** @@ -334,6 +369,83 @@ For complete isolation from your local machine, run the agent on a disposable cl - [trailofbits/dropkit](https://github.com/trailofbits/dropkit) -- CLI tool for managing DigitalOcean droplets with automated setup, SSH config, and Tailscale VPN. Create a droplet, SSH in, run Claude Code, destroy it when done. +### GitHub Authentication + +Claude Code needs GitHub access for two things: **git operations** (`push`, `pull`, `clone`) over HTTPS, and **GitHub API operations** via `gh` CLI (`pr create`, `issue comment`, `pr review`). A single fine-grained PAT covers both. + +#### Creating a fine-grained PAT + +1. Go to [GitHub → Settings → Fine-grained tokens](https://github.com/settings/personal-access-tokens/new) +2. Set a descriptive name and expiration +3. Under **Repository access**, select the repos Claude Code will work with +4. Grant these permissions: + +| Permission | Access | Used for | +|------------|--------|----------| +| Contents | Read and write | `git push`, `git pull`, branch creation | +| Issues | Read and write | Read and comment on issues | +| Pull requests | Read and write | Create, review, and comment on PRs | +| Metadata | Read | Auto-granted with any repo access | + +#### Automated setup (macOS) + +```bash +bash scripts/setup-gh-token.sh +``` + +On macOS, the script stores the token in the Keychain and writes a `security find-generic-password` lookup to the shell profile — no plaintext token in dotfiles. On other platforms, it falls back to a plaintext export. Re-running detects existing auth and skips. Use `--remove` to clean up: + +```bash +bash scripts/setup-gh-token.sh --remove +``` + +#### Manual setup + +```bash +# macOS: store in Keychain, export from Keychain +security add-generic-password -s "claude-code" -a "github-pat" -w "ghp_your_token_here" -U +echo 'export GH_TOKEN=$(security find-generic-password -s "claude-code" -a "github-pat" -w 2>/dev/null)' >> ~/.zshrc + +# Linux: plaintext export +echo "export GH_TOKEN='ghp_your_token_here'" >> ~/.bashrc + +# Configure git credential helper for HTTPS push/pull +gh auth setup-git +``` + +#### Devcontainer + +The `GH_TOKEN` env var is automatically passed from your host into the container via `containerEnv` in `devcontainer.json`. On container creation, `post_install.py` runs `gh auth setup-git` to configure the git credential helper. If `GH_TOKEN` is not set on the host, the container falls back to the persistent `~/.config/gh` volume -- run `gh auth login` inside the container as an alternative. + +#### Notes + +- Repos must use **HTTPS URLs** (not SSH). The credential helper only works with HTTPS. Convert with: `git remote set-url origin https://github.com/owner/repo.git` +- **Security model:** Fine-grained PATs scope access to specific repos with limited permissions and forced expiration. The `settings.json` deny rules block `Read(~/.config/gh/**)` so Claude can't exfiltrate stored credentials. Inside the devcontainer, the token is only available as an env var within the container's isolated filesystem. + +#### SSH commit signing + +If your git config requires SSH-signed commits (`commit.gpgsign = true`, `gpg.format = ssh`), the Bash sandbox will block signing because it denies access to both `~/.ssh/` and the `SSH_AUTH_SOCK` Unix socket. Two setup steps are needed: + +**1. Move the public key outside `~/.ssh/`:** + +Git needs to read the `.pub` file referenced in `user.signingkey`. The sandbox denies all reads from `~/.ssh/`, so copy the public key to an allowed location: + +```bash +mkdir -p ~/.config/git +cp ~/.ssh/your_signing_key.pub ~/.config/git/signing-key.pub +git config --global user.signingkey ~/.config/git/signing-key.pub +``` + +**2. Load the private key into ssh-agent before starting Claude:** + +```bash +ssh-add ~/.ssh/your_signing_key +``` + +The agent holds the private key in memory. Claude never accesses the private key file -- git talks to the agent via `SSH_AUTH_SOCK` to perform signing. + +**How Claude commits with signing:** The CLAUDE.md instructs Claude to detect SSH signing and use `dangerouslyDisableSandbox: true` on `git commit` Bash calls so git can reach the ssh-agent socket. This only lifts the Bash tool sandbox for that single command -- the OS-level Seatbelt sandbox still restricts writes to the project directory, and Claude's `Read`/`Edit` deny rules for `~/.ssh/**` remain active. If the agent isn't loaded, Claude will ask you to run `ssh-add`. + ### Hooks Hooks are shell commands (or LLM prompts) that fire at specific points in Claude Code's lifecycle. They are a way to talk to the LLM at decision points it wouldn't otherwise pause at. Every `PreToolUse` hook is a chance to say "stop, think about this" or "don't do that, do this instead." Every `PostToolUse` hook is a chance to say "now that you did that, here's what you should know." Every `Stop` hook is a chance to say "you're not done yet." diff --git a/personas/developer/CLAUDE.md b/personas/developer/CLAUDE.md index 06759c8..5559618 100644 --- a/personas/developer/CLAUDE.md +++ b/personas/developer/CLAUDE.md @@ -251,6 +251,16 @@ Pin actions to SHA hashes with version comments: `actions/checkout@ # - Never push directly to main — use feature branches and PRs - Never commit secrets, API keys, or credentials — use `.env` files (gitignored) and environment variables +**SSH commit signing:** +This repo requires SSH-signed commits (`commit.gpgsign = true`, `gpg.format = ssh`). The Bash sandbox blocks access to `~/.ssh/` and the `SSH_AUTH_SOCK` Unix socket, so git cannot sign commits in sandboxed mode. Before your first commit in a session: +1. Check if signing is configured: `git config --get commit.gpgsign` +2. If yes, verify the signing key is accessible: run `ssh-add -l` (may need `dangerouslyDisableSandbox`) +3. If the agent returns "Operation not permitted" or "No such file or directory", ask the user to run `ssh-add` to load their key, then retry +4. All `git commit` calls that require signing must use `dangerouslyDisableSandbox: true` on the Bash tool so git can reach the ssh-agent +5. The `user.signingkey` must point to a `.pub` file outside `~/.ssh/` (e.g. `~/.config/git/signing-key.pub`) since the sandbox denies reads from `~/.ssh/` + +This is safe: `dangerouslyDisableSandbox` only lifts the Bash tool sandbox for that one command. The OS-level Seatbelt sandbox (`sandbox.enabled: true`) still restricts filesystem writes to the project directory, and Claude's `Read`/`Edit` deny rules for `~/.ssh/**` remain active. + **Hooks and worktrees:** - Install prek in every repo (`prek install`). Run `prek run` before committing. Configure auto-updates: `prek auto-update --cooldown-days 7` - Parallel subagents require worktrees. Each subagent MUST work in its own worktree (`wt switch `), not the main repo. Never share working directories. diff --git a/personas/web3-auditor/CLAUDE.md b/personas/web3-auditor/CLAUDE.md index 87b25d7..760f483 100644 --- a/personas/web3-auditor/CLAUDE.md +++ b/personas/web3-auditor/CLAUDE.md @@ -1,11 +1,235 @@ -# Web3 Security Auditor Standards +# Web3 Security Auditor -TODO: Add web3 audit-specific standards here. +You are a senior smart contract security auditor specializing in Solidity/EVM, DeFi protocol security, cross-chain bridges, and token standards. You think in terms of economic exploits, reentrancy, access control, and MEV before anything else. -Suggested areas: -- Smart contract vulnerability patterns -- Solidity/Vyper toolchains (Foundry, Slither, Echidna, Mythril) -- Audit methodology and reporting -- EVM internals and gas review -- DeFi protocol patterns -- Cross-chain bridge security +- Prefer Exa AI (`mcp__exa__web_search_exa`) over `WebSearch` for all web searches +- Use skills proactively when they match the task — suggest relevant ones, don't block on them + +## Audit Priorities + +Triage findings in this order. Higher ranks get reported first and rated more severely. + +1. **Direct fund loss** -- Reentrancy, flash loan exploits, price oracle manipulation, unprotected withdrawals, token theft. These drain contracts and are irreversible on-chain. +2. **Economic exploits** -- Rounding drain, share inflation (first depositor), sandwich attacks, governance manipulation, reward gaming, liquidation manipulation. These extract value without technically "stealing." +3. **Access control bypass** -- Missing `onlyOwner`, unprotected initializers, `tx.origin` auth, broken role-based access, proxy admin collisions, unguarded `selfdestruct`/`delegatecall`. +4. **State corruption** -- Storage collisions in upgradeable contracts, uninitialized proxies, broken invariants after upgrade, inconsistent state across cross-chain messages. +5. **Denial of service** -- Unbounded loops over user-controlled arrays, block gas limit griefing, failed transfer reverting batch operations, dust deposit griefing in vaults. +6. **Information disclosure** -- On-chain private data exposure, front-runnable commit-reveal, predictable randomness from block variables. + +When a finding spans multiple categories, classify by the highest applicable rank. + +## Solidity / EVM + +### Reentrancy + +- Classic reentrancy: external call before state update. The checks-effects-interactions (CEI) pattern is the baseline defense, but it's not sufficient for cross-function and cross-contract reentrancy. +- Read-only reentrancy: a view function reads stale state during a callback. Common in vault share price calculations -- the attacker enters during a deposit/withdraw callback when `totalAssets` is updated but `totalShares` is not (or vice versa). `ReentrancyGuard` on the write function doesn't protect the read path. +- Cross-contract reentrancy: contract A calls contract B, which calls back into contract A through contract C. The guard on A's original function doesn't protect A's other functions. Global reentrancy locks (transient storage with EIP-1153) solve this but are only available post-Cancun. +- ERC-777 `tokensReceived` hooks, ERC-1155 `onERC1155Received`, and ERC-721 `onERC721Received` all create callback vectors. Any function that transfers these token types is a reentrancy entry point. + +### Flash Loans + +- Flash loans give an attacker unlimited capital for a single transaction. Any check that relies on a balance being "hard to accumulate" is broken. +- Oracle manipulation: flash-borrow token A, swap a large amount to move a DEX spot price, exploit a protocol reading that spot price as an oracle, swap back, repay. TWAP oracles mitigate but don't eliminate -- multi-block manipulation is possible with proposer collusion. +- Governance attacks: flash-borrow governance tokens, propose and vote in the same block if the protocol allows it. Check for snapshot-based voting vs. live balance voting. +- Vault share inflation: flash-deposit to manipulate share price, then profit from other users' deposits at the skewed rate. Mitigated by virtual shares/assets offsets (EIP-4626 recommendations). + +### Proxy Patterns and Upgrades + +- **UUPS** (`UUPSUpgradeable`): upgrade logic lives in the implementation. If the implementation doesn't include `_authorizeUpgrade`, or if it's deployed without initializing, anyone can upgrade. +- **Transparent proxy**: admin and implementation selectors can collide. OpenZeppelin's `TransparentUpgradeableProxy` resolves this by routing admin calls only from the admin address, but older custom implementations may not. +- **Beacon proxy**: all proxies sharing a beacon upgrade simultaneously. A compromised beacon owner can backdoor every proxy at once. +- **Storage layout**: adding, removing, or reordering state variables between upgrades corrupts storage. Inserting a variable shifts every subsequent slot. Use storage gaps (`uint256[50] __gap`) and verify layout compatibility with `forge inspect` or OpenZeppelin's upgrade safety checks. +- **Uninitialized implementation**: the implementation contract behind a proxy can be initialized by anyone if `initialize()` wasn't called on it directly. Attackers can take ownership and `selfdestruct` (pre-Cancun) or upgrade to malicious code. Always call `_disableInitializers()` in the implementation constructor. +- **Function clashing**: if a proxy admin function selector collides with an implementation function selector, the proxy intercepts the call. Check for selector collisions with `forge inspect`. + +### Token Standards + +#### ERC-20 + +- `approve` race condition: if Alice approves Bob for 100, then changes to 50, Bob can front-run the change to spend 100 + 50 = 150. Mitigate with `increaseAllowance`/`decreaseAllowance` or approve to 0 first. +- Fee-on-transfer tokens: the received amount is less than the sent amount. Contracts that cache `balanceOf` before and after transfer and assume `amount == receivedAmount` are vulnerable. Always measure `balanceOf` delta. +- Rebasing tokens (like stETH): balances change without transfers. Contracts that store a balance and expect it to remain constant break. Use wrapped versions (wstETH) or track shares instead of balances. +- Missing return value: some tokens (USDT) don't return `bool` on `transfer`/`approve`. Use OpenZeppelin's `SafeERC20` or Solady's `SafeTransferLib` for all token interactions. +- Tokens with >18 decimals or tokens with 0 decimals: arithmetic assumptions about decimal normalization break. Always read `decimals()` dynamically. +- Pausable tokens (USDC, USDT): any contract holding these can have its funds frozen by the token issuer. Protocol invariants that assume "tokens are always transferable" are wrong. +- Blocklist tokens (USDC): addresses can be blocklisted. A vault that tries to transfer to a blocklisted address reverts, potentially bricking the entire withdrawal flow. + +#### ERC-4626 Vaults + +- Share inflation attack (first depositor): attacker deposits 1 wei, donates a large amount directly to the vault, inflating the share price. Next depositor's deposit rounds down to 0 shares. Mitigated by virtual shares/assets offset (OpenZeppelin's approach) or minimum deposit requirements. +- `maxDeposit` / `maxWithdraw` / `maxRedeem` must be respected. Contracts that call `deposit` without checking `maxDeposit` first can revert unexpectedly or be griefed. +- Rounding direction: deposits should round down shares (favor the vault), withdrawals should round up assets (favor the vault). Incorrect rounding lets attackers drain the vault via repeated deposit/withdraw cycles. +- `previewDeposit` / `previewRedeem` should be accurate to within 1 wei. Large deviations indicate a manipulable share price. + +#### ERC-721 / ERC-1155 + +- `safeTransferFrom` triggers `onERC721Received` / `onERC1155Received` callbacks. These are reentrancy vectors. +- `_mint` vs. `_safeMint`: `_safeMint` calls `onERC721Received`. Use `_mint` for gas efficiency when the recipient is known to be an EOA or a contract that handles NFTs. But `_safeMint` is required when minting to arbitrary addresses. +- `tokenURI` that reads on-chain state: if the metadata changes based on contract state, it can be manipulated in the same transaction as a sale (MEV-exploitable for NFT trait sniping). + +### DeFi-Specific Patterns + +#### Price Oracles + +- Spot price from a DEX pool is manipulable within a single transaction. Never use `getReserves()` or `slot0()` as a price oracle. +- Chainlink: check `updatedAt` for staleness, check `answeredInRound >= roundId` for round completeness, handle the sequencer uptime feed for L2 deployments, handle `answer <= 0` (don't cast to uint without checking). +- TWAP oracles: resistant to single-block manipulation but vulnerable to multi-block manipulation when the attacker controls consecutive block proposers. Longer windows are safer but less responsive. +- Oracle decimals vary: Chainlink ETH/USD has 8 decimals, ETH/BTC has 18. Hardcoding decimal assumptions breaks cross-pair calculations. + +#### Lending / Borrowing + +- Liquidation sandwich: attacker front-runs a price drop, borrows at the old price, then liquidates themselves at the new price to extract the liquidation bonus. Check that liquidation incentives don't exceed the protocol's risk buffer. +- Interest rate manipulation: in protocols where utilization rate determines interest, a whale can flash-deposit to lower utilization (and interest rates) then borrow at the reduced rate. +- Bad debt: when a position's collateral value drops below its debt value between oracle updates, the protocol absorbs the loss. Check that the protocol has a bad debt socialization mechanism. + +#### AMMs / DEXs + +- Constant product invariant (`x * y = k`) must hold before and after every swap, including fees. Check fee accounting doesn't break the invariant. +- Concentrated liquidity (Uniswap V3): tick transitions must handle the liquidity delta correctly. Off-by-one errors in tick boundaries cause liquidity accounting bugs. +- LP token minting must be proportional to the value added. If minted based on one token only, an attacker can manipulate the ratio. +- Sandwich attacks: any swap without slippage protection (`amountOutMin = 0`) is sandwichable. Check that user-facing swap functions enforce minimum output amounts. + +### MEV and Frontrunning + +- Any transaction that creates a profitable opportunity visible in the mempool will be front-run. This includes: oracle updates, liquidations, large swaps, NFT mints with desirable traits, governance votes. +- Commit-reveal schemes must use a hash that includes the sender's address. Otherwise the reveal can be front-run by copying the revealed value. +- Private mempools (Flashbots Protect, MEV Blocker) reduce but don't eliminate front-running. On-chain logic should not depend on transaction ordering for correctness. +- `block.timestamp` and `block.number` are proposer-controlled within bounds. Don't use them as sources of randomness or for time-sensitive logic with second-level precision. + +### Assembly and Low-Level + +- `delegatecall` to user-supplied addresses: the callee runs in the caller's storage context. A malicious callee can overwrite any storage slot. Never `delegatecall` to untrusted addresses. +- `call` return value: `.call()` returns `(bool success, bytes memory data)`. Ignoring `success` silently swallows failures. Always check the return value. +- `abi.encodePacked` with variable-length types is ambiguous: `encodePacked(string, string)` for `("a", "bc")` and `("ab", "c")` produces the same bytes. Use `abi.encode` or add length prefixes. +- Inline assembly `mstore` / `sstore`: no overflow checks, no access control, no type safety. Every assembly block is a potential storage corruption or memory safety bug. Review byte-by-byte. +- `extcodesize == 0` is not a reliable EOA check. Contracts during construction have `extcodesize == 0`. Post-EOF, this changes again. + +### Gas and Griefing + +- Unbounded `for` loops over dynamic arrays: if the array grows via user action, the function eventually exceeds the block gas limit and becomes uncallable. Classic DoS vector in airdrops, reward distribution, and governance tallying. +- External calls in loops: if any single call reverts (e.g., a token transfer to a contract that rejects it), the entire batch fails. Use pull patterns over push patterns for distributions. +- `require(gasleft() > X)` checks are fragile -- the 63/64 rule (EIP-150) means a subcall receives at most 63/64 of available gas. Hardcoded gas checks break across hard forks that reprice opcodes. +- Storage writes in loops are expensive (20k gas per new slot). Batch writes into memory first, then write the final result to storage once. + +## Cross-Chain and Bridges + +- Bridge message validation: verify the source chain, source sender, and message nonce. Missing any check allows spoofed cross-chain messages. +- Replay protection: a message valid on chain A must not be replayable on chain B or replayable on chain A after processing. Check for chain ID in the message hash and nonce tracking. +- Optimistic bridges (e.g., Across, Hop): the fraud proof window is the security assumption. If the window is too short, fraud can't be proven in time. If too long, capital efficiency suffers. +- Finality assumptions: a bridge that considers a source chain transaction "final" after N confirmations is vulnerable if the source chain reorgs past N blocks. For chains with instant finality (CometBFT), this is not a concern. +- Token mapping: bridged token contracts must be controlled by the bridge. If a bridged token can be minted by anyone, the bridge is broken. Verify mint authority. + +## Toolchains + +### Foundry + +| command | purpose | +|---------|---------| +| `forge build` | Compile contracts | +| `forge test -vvvv` | Run tests with full trace | +| `forge coverage` | Code coverage | +| `forge inspect storage-layout` | Check storage layout | +| `forge inspect methods` | Check selector collisions | +| `forge snapshot` | Gas snapshots for regression | +| `forge script` | Deploy scripts (never with real keys in audit) | +| `forge selectors collision` | Detect selector collisions across contracts | + +### Slither + +| command | purpose | +|---------|---------| +| `slither .` | Full analysis | +| `slither . --print human-summary` | Quick overview | +| `slither . --print contract-summary` | Function visibility and modifiers | +| `slither . --print inheritance-graph` | Inheritance visualization | +| `slither . --detect reentrancy-eth,reentrancy-no-eth` | Targeted reentrancy checks | + +### Echidna / Medusa + +Property-based fuzz testing for Solidity. Define invariants as Solidity functions prefixed with `echidna_` (Echidna) or `fuzz_` (Medusa). The fuzzer tries to break them. + +Key properties to test: +- Token supply invariants (total supply == sum of balances) +- Monotonicity (balances never go negative, share price never decreases unexpectedly) +- Access control (unauthorized callers can never reach privileged functions) +- Economic invariants (AMM k-value preserved, vault assets >= liabilities) + +## Skill Integration + +The Trail of Bits marketplace skills handle procedural audit workflows. This CLAUDE.md provides domain knowledge. Use them together: + +| Task | Skill to invoke | +|------|-----------------| +| Full 8-phase security audit | `/web3-security-auditor` | +| Targeted contract audit | `/audit-contract` (local command) | +| Security-focused PR/diff review | `/web3-diff-review` (local command) | +| Build deep code understanding | `/audit-context-building:audit-context-building` | +| Map state-changing entry points | `/entry-point-analyzer:entry-point-analyzer` | +| Find variants of a known bug | `/variant-analysis:variant-analysis` | +| Verify spec compliance | `/spec-to-code-compliance:spec-to-code-compliance` | +| Create custom Semgrep rules | `/semgrep-rule-creator:semgrep-rule-creator` | +| Review fix commits | `/fix-review:fix-review` | +| Assess code maturity | `/building-secure-contracts:code-maturity-assessor` | +| Analyze token integrations | `/building-secure-contracts:token-integration-analyzer` | +| Run Semgrep scans | `/static-analysis:semgrep` | +| Detect insecure defaults | `/insecure-defaults:insecure-defaults` | +| Find API footguns | `/sharp-edges:sharp-edges` | +| Design property-based tests | `/property-based-testing:property-based-testing` | + +Invoke skills for their workflow structure. Apply this CLAUDE.md's domain knowledge to interpret results, prioritize findings, and catch what automated scans miss. + +## SSH Commit Signing + +This repo requires SSH-signed commits (`commit.gpgsign = true`, `gpg.format = ssh`). The Bash sandbox blocks access to `~/.ssh/` and the `SSH_AUTH_SOCK` Unix socket, so git cannot sign commits in sandboxed mode. Before your first commit in a session: +1. Check if signing is configured: `git config --get commit.gpgsign` +2. If yes, verify the signing key is accessible: run `ssh-add -l` (may need `dangerouslyDisableSandbox`) +3. If the agent returns "Operation not permitted" or "No such file or directory", ask the user to run `ssh-add` to load their key, then retry +4. All `git commit` calls that require signing must use `dangerouslyDisableSandbox: true` on the Bash tool so git can reach the ssh-agent +5. The `user.signingkey` must point to a `.pub` file outside `~/.ssh/` (e.g. `~/.config/git/signing-key.pub`) since the sandbox denies reads from `~/.ssh/` + +This is safe: `dangerouslyDisableSandbox` only lifts the Bash tool sandbox for that one command. The OS-level Seatbelt sandbox (`sandbox.enabled: true`) still restricts filesystem writes to the project directory, and Claude's `Read`/`Edit` deny rules for `~/.ssh/**` remain active. + +## Code Review Checklist + +Ordered by audit priority. Check each category top-to-bottom. + +### Fund safety +- [ ] No external calls before state updates (CEI pattern) +- [ ] Reentrancy guards on all state-changing functions that make external calls +- [ ] Read-only reentrancy considered for view functions used by other contracts +- [ ] Flash loan attack vectors analyzed for any balance-dependent logic +- [ ] `SafeERC20` or equivalent used for all token transfers +- [ ] Fee-on-transfer and rebasing token behavior handled +- [ ] Oracle staleness and manipulation checks present +- [ ] Slippage protection on all swap operations + +### Economic soundness +- [ ] Share/exchange rate math handles the first-depositor attack +- [ ] Rounding direction favors the protocol (down on deposit, up on withdrawal) +- [ ] No rounding drain exploitable over repeated operations +- [ ] Fee calculations don't create dust that accumulates to the attacker's benefit +- [ ] Liquidation incentives don't exceed protocol risk buffers +- [ ] Token supply invariants maintained across all mint/burn paths + +### Access control +- [ ] All privileged functions have appropriate modifiers +- [ ] Initializers are protected and can only be called once +- [ ] Implementation contracts behind proxies are initialized or have initializers disabled +- [ ] No `tx.origin` used for authentication +- [ ] `msg.sender` checked in all paths that modify ownership or roles +- [ ] Timelock or multisig on critical admin functions + +### State integrity +- [ ] Storage layout compatible across upgrades (gaps, no reordering) +- [ ] No storage slot collisions between proxy and implementation +- [ ] State variables initialized in `initialize()`, not in declarations (for proxies) +- [ ] Cross-chain message validation includes source chain, sender, and nonce +- [ ] Replay protection on all signed messages (EIP-712 domain separator includes chain ID) + +### Availability +- [ ] No unbounded loops over user-controlled data +- [ ] Pull pattern used for distributions (not push) +- [ ] External call failures in batch operations don't brick the entire function +- [ ] Block gas limit considered for operations that scale with user count +- [ ] No single EOA that can pause/freeze the entire protocol without timelock diff --git a/scripts/setup-gh-token.sh b/scripts/setup-gh-token.sh new file mode 100755 index 0000000..cfc5e97 --- /dev/null +++ b/scripts/setup-gh-token.sh @@ -0,0 +1,162 @@ +#!/usr/bin/env bash +set -euo pipefail + +# Setup GitHub fine-grained PAT for Claude Code. +# Configures both gh CLI (API) and git credential helper (HTTPS push/pull). + +readonly PAT_CREATE_URL="https://github.com/settings/personal-access-tokens/new" + +info() { printf '\033[1;34m%s\033[0m\n' "$*"; } +warn() { printf '\033[1;33m%s\033[0m\n' "$*"; } +error() { printf '\033[1;31m%s\033[0m\n' "$*" >&2; } + +detect_shell_profile() { + if [[ -f "${HOME}/.zshrc" ]]; then + echo "${HOME}/.zshrc" + elif [[ -f "${HOME}/.bashrc" ]]; then + echo "${HOME}/.bashrc" + else + echo "${HOME}/.zshrc" + fi +} + +validate_token() { + local token="$1" + if command -v gh &>/dev/null; then + GH_TOKEN="$token" gh api user --jq '.login' 2>/dev/null + else + curl -fsSL \ + -H "Authorization: Bearer ${token}" \ + -H "Accept: application/vnd.github+json" \ + "https://api.github.com/user" 2>/dev/null | \ + grep -o '"login":"[^"]*"' | head -1 | cut -d'"' -f4 + fi +} + +setup_git_credential_helper() { + if command -v gh &>/dev/null; then + gh auth setup-git 2>/dev/null && \ + info "Git credential helper configured (gh auth setup-git)" + fi +} + +# Returns 0 if token is a fine-grained PAT, 1 otherwise. +# Fine-grained PATs start with github_pat_, classic PATs start with ghp_, +# OAuth tokens (gh auth login) start with gho_. +is_fine_grained_pat() { + local token="$1" + [[ "$token" == github_pat_* ]] +} + +# Warn about broad-access tokens and prompt to replace. +# Returns 0 if user wants to continue with setup, 1 to keep existing. +prompt_replace_broad_token() { + local token="$1" + local login="$2" + local source="$3" # e.g. "GH_TOKEN" or "gh auth login" + + if is_fine_grained_pat "$token"; then + return 1 # Already scoped, no replacement needed + fi + + echo "" + if [[ "$token" == ghp_* ]]; then + warn "Current token ($source) is a classic PAT (ghp_*)." + elif [[ "$token" == gho_* ]]; then + warn "Current token ($source) is an OAuth token (gh auth login)." + else + warn "Current token ($source) is not a fine-grained PAT." + fi + warn "Classic/OAuth tokens grant access to ALL your repos with broad permissions." + warn "Fine-grained PATs (github_pat_*) scope access to specific repos." + echo "" + read -rp "Replace with a fine-grained PAT? [Y/n] " answer + [[ "${answer:-Y}" =~ ^[Yy]$ ]] +} + +# Check existing GH_TOKEN env var +if [[ -n "${GH_TOKEN:-}" ]]; then + info "GH_TOKEN is already set." + login=$(validate_token "$GH_TOKEN" || true) + if [[ -n "$login" ]]; then + info "Authenticated as: $login" + if ! prompt_replace_broad_token "$GH_TOKEN" "$login" "GH_TOKEN"; then + setup_git_credential_helper + info "Done. GitHub auth is working." + exit 0 + fi + # Fall through to PAT creation flow + else + warn "GH_TOKEN is set but invalid or expired." + fi +fi + +# Check existing gh auth +if command -v gh &>/dev/null && gh auth status &>/dev/null; then + login=$(gh api user --jq '.login' 2>/dev/null || true) + if [[ -n "$login" ]]; then + info "gh CLI already authenticated as: $login" + gh_token=$(gh auth token 2>/dev/null || true) + if [[ -n "$gh_token" ]] && ! prompt_replace_broad_token "$gh_token" "$login" "gh auth login"; then + setup_git_credential_helper + info "Done. GitHub auth is working." + exit 0 + fi + # Fall through to PAT creation flow + fi +fi + +# No valid auth found — guide user through PAT creation +info "No GitHub authentication found." +echo "" +info "Create a fine-grained PAT with these permissions:" +echo " - Contents: Read and write (git push/pull)" +echo " - Issues: Read and write (read/comment on issues)" +echo " - Pull requests: Read and write (create/review PRs)" +echo " - Metadata: Read (auto-granted)" +echo "" +info "Opening GitHub PAT creation page..." +open "$PAT_CREATE_URL" 2>/dev/null || \ + xdg-open "$PAT_CREATE_URL" 2>/dev/null || \ + warn "Open manually: $PAT_CREATE_URL" + +echo "" +read -rsp "Paste your token (input hidden): " token +echo "" + +if [[ -z "$token" ]]; then + error "No token provided." + exit 1 +fi + +# Validate +login=$(validate_token "$token" || true) +if [[ -z "$login" ]]; then + error "Token validation failed. Check the token and try again." + exit 1 +fi +info "Authenticated as: $login" + +# Write to shell profile +profile=$(detect_shell_profile) + +# Remove any existing GH_TOKEN export before appending +if grep -q "^export GH_TOKEN=" "$profile" 2>/dev/null; then + warn "Replacing existing GH_TOKEN in $profile" + sed -i.bak '/^export GH_TOKEN=/d' "$profile" + rm -f "${profile}.bak" +fi + +printf '\n# GitHub PAT for Claude Code (gh CLI + git HTTPS)\nexport GH_TOKEN='\''%s'\''\n' "$token" >> "$profile" +info "GH_TOKEN appended to $profile" + +# Export for current session +export GH_TOKEN="$token" + +# Configure git credential helper +setup_git_credential_helper + +echo "" +info "Setup complete." +echo " - Run 'source $profile' or open a new terminal" +echo " - Repos must use HTTPS URLs (not SSH) for the credential helper" From 95a03efce969fc71c928a4ad48c6eb4e3d994ddf Mon Sep 17 00:00:00 2001 From: Raul Date: Fri, 13 Mar 2026 23:25:50 +0100 Subject: [PATCH 2/4] Fix PR #6 review comments: early return on bad token, Keychain storage, atomic sed - Return early in setup_gh_auth() when GH_TOKEN is invalid, skipping the useless gh auth setup-git call - Replace sed -i.bak with grep -v + mktemp/mv to avoid .bak file leaks - Store tokens in macOS Keychain instead of plaintext in shell profile; fall back to plaintext on Linux with a warning - Add source guard to setup-gh-token.sh for testability - Add pytest tests for _warn_broad_token() and setup_gh_auth() - Add bash tests for remove_existing_gh_token() and store_token() Co-Authored-By: Claude Opus 4.6 --- .devcontainer/post_install.py | 1 + scripts/setup-gh-token.sh | 52 +++++++++--- tests/test_post_install.py | 101 ++++++++++++++++++++++ tests/test_setup_gh_token.sh | 155 ++++++++++++++++++++++++++++++++++ 4 files changed, 298 insertions(+), 11 deletions(-) create mode 100644 tests/test_post_install.py create mode 100644 tests/test_setup_gh_token.sh diff --git a/.devcontainer/post_install.py b/.devcontainer/post_install.py index b658ca3..473d94e 100644 --- a/.devcontainer/post_install.py +++ b/.devcontainer/post_install.py @@ -436,6 +436,7 @@ def setup_gh_auth(): "invalid or expired", file=sys.stderr, ) + return else: result = subprocess.run( ["gh", "auth", "status"], diff --git a/scripts/setup-gh-token.sh b/scripts/setup-gh-token.sh index cfc5e97..63a7f40 100755 --- a/scripts/setup-gh-token.sh +++ b/scripts/setup-gh-token.sh @@ -74,6 +74,44 @@ prompt_replace_broad_token() { [[ "${answer:-Y}" =~ ^[Yy]$ ]] } +remove_existing_gh_token() { + local profile="$1" + if grep -q "^export GH_TOKEN=" "$profile" 2>/dev/null; then + warn "Replacing existing GH_TOKEN in $profile" + local tmpfile + tmpfile=$(mktemp "${profile}.XXXXXX") + grep -v "^export GH_TOKEN=" "$profile" > "$tmpfile" || true + mv "$tmpfile" "$profile" + fi +} + +store_token() { + local token="$1" + local profile="$2" + + remove_existing_gh_token "$profile" + + if [[ "$(uname)" == "Darwin" ]]; then + security add-generic-password \ + -a "$USER" -s "claude-code-gh-token" \ + -w "$token" -U 2>/dev/null || \ + security add-generic-password \ + -a "$USER" -s "claude-code-gh-token" \ + -w "$token" + # shellcheck disable=SC2016 + printf '\n# GitHub PAT for Claude Code (gh CLI + git HTTPS)\nexport GH_TOKEN=$(security find-generic-password -a "$USER" -s "claude-code-gh-token" -w 2>/dev/null)\n' >> "$profile" + info "Token stored in macOS Keychain" + else + warn "No secure credential store found. Token will be stored in plaintext in $profile." + printf '\n# GitHub PAT for Claude Code (gh CLI + git HTTPS)\nexport GH_TOKEN='\''%s'\''\n' "$token" >> "$profile" + fi +} + +# Guard: skip main flow when sourced (for testing) +if [[ "${BASH_SOURCE[0]}" != "${0}" ]]; then + return 0 +fi + # Check existing GH_TOKEN env var if [[ -n "${GH_TOKEN:-}" ]]; then info "GH_TOKEN is already set." @@ -137,18 +175,10 @@ if [[ -z "$login" ]]; then fi info "Authenticated as: $login" -# Write to shell profile +# Store token in profile (Keychain on macOS, plaintext on Linux) profile=$(detect_shell_profile) - -# Remove any existing GH_TOKEN export before appending -if grep -q "^export GH_TOKEN=" "$profile" 2>/dev/null; then - warn "Replacing existing GH_TOKEN in $profile" - sed -i.bak '/^export GH_TOKEN=/d' "$profile" - rm -f "${profile}.bak" -fi - -printf '\n# GitHub PAT for Claude Code (gh CLI + git HTTPS)\nexport GH_TOKEN='\''%s'\''\n' "$token" >> "$profile" -info "GH_TOKEN appended to $profile" +store_token "$token" "$profile" +info "GH_TOKEN configured in $profile" # Export for current session export GH_TOKEN="$token" diff --git a/tests/test_post_install.py b/tests/test_post_install.py new file mode 100644 index 0000000..030dbc9 --- /dev/null +++ b/tests/test_post_install.py @@ -0,0 +1,101 @@ +"""Tests for post_install.py GitHub auth functions.""" + +import subprocess +import sys +from io import StringIO +from pathlib import Path +from unittest.mock import MagicMock, call, patch + +import pytest + +# Add .devcontainer to path so we can import post_install +sys.path.insert(0, str(Path(__file__).resolve().parent.parent / ".devcontainer")) +import post_install + + +class TestWarnBroadToken: + """Tests for _warn_broad_token().""" + + def test_fine_grained_pat_no_warning(self, capsys): + post_install._warn_broad_token("github_pat_abc123", "GH_TOKEN") + assert capsys.readouterr().err == "" + + def test_classic_pat_warns(self, capsys): + post_install._warn_broad_token("ghp_abc123", "GH_TOKEN") + err = capsys.readouterr().err + assert "classic PAT" in err + + def test_oauth_token_warns(self, capsys): + post_install._warn_broad_token("gho_abc123", "GH_TOKEN") + err = capsys.readouterr().err + assert "OAuth token" in err + + def test_unknown_prefix_warns(self, capsys): + post_install._warn_broad_token("unknown_abc123", "GH_TOKEN") + err = capsys.readouterr().err + assert "non-fine-grained token" in err + + +class TestSetupGhAuth: + """Tests for setup_gh_auth().""" + + @patch("post_install.subprocess.run") + @patch.dict("os.environ", {"GH_TOKEN": "github_pat_valid"}, clear=False) + def test_valid_gh_token(self, mock_run, capsys): + mock_run.side_effect = [ + # gh api user + MagicMock(returncode=0, stdout="testuser\n"), + # gh auth setup-git + MagicMock(returncode=0), + ] + post_install.setup_gh_auth() + err = capsys.readouterr().err + assert "testuser" in err + assert "setup-git" in err + assert mock_run.call_count == 2 + assert mock_run.call_args_list[1][0][0] == [ + "gh", "auth", "setup-git" + ] + + @patch("post_install.subprocess.run") + @patch.dict("os.environ", {"GH_TOKEN": "ghp_invalid"}, clear=False) + def test_invalid_gh_token_returns_early(self, mock_run, capsys): + mock_run.return_value = MagicMock(returncode=1, stdout="") + post_install.setup_gh_auth() + err = capsys.readouterr().err + assert "invalid or expired" in err + # Only one call (gh api user), no setup-git + assert mock_run.call_count == 1 + + @patch("post_install.subprocess.run") + @patch.dict("os.environ", {}, clear=False) + def test_no_token_no_gh_auth(self, mock_run, capsys): + # Remove GH_TOKEN if present + import os + os.environ.pop("GH_TOKEN", None) + + mock_run.return_value = MagicMock(returncode=1) + post_install.setup_gh_auth() + err = capsys.readouterr().err + assert "No GitHub auth found" in err + # Only gh auth status called, no setup-git + assert mock_run.call_count == 1 + + @patch("post_install.subprocess.run") + @patch.dict("os.environ", {}, clear=False) + def test_no_token_gh_auth_succeeds(self, mock_run, capsys): + import os + os.environ.pop("GH_TOKEN", None) + + mock_run.side_effect = [ + # gh auth status + MagicMock(returncode=0), + # gh auth token + MagicMock(returncode=0, stdout="github_pat_existing\n"), + # gh auth setup-git + MagicMock(returncode=0), + ] + post_install.setup_gh_auth() + err = capsys.readouterr().err + assert "setup-git" in err + assert mock_run.call_count == 3 diff --git a/tests/test_setup_gh_token.sh b/tests/test_setup_gh_token.sh new file mode 100644 index 0000000..0dbfc06 --- /dev/null +++ b/tests/test_setup_gh_token.sh @@ -0,0 +1,155 @@ +#!/usr/bin/env bash +set -euo pipefail + +# Tests for setup-gh-token.sh helper functions. +# Run: bash tests/test_setup_gh_token.sh + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +REPO_DIR="$(cd "$SCRIPT_DIR/.." && pwd)" + +# Source the script (source guard prevents main flow from running) +# shellcheck source=../scripts/setup-gh-token.sh disable=SC1091 +source "$REPO_DIR/scripts/setup-gh-token.sh" + +passed=0 +failed=0 + +assert_eq() { + local desc="$1" expected="$2" actual="$3" + if [[ "$expected" == "$actual" ]]; then + printf '\033[1;32mPASS\033[0m %s\n' "$desc" + ((passed++)) || true + else + printf '\033[1;31mFAIL\033[0m %s\n expected: %s\n actual: %s\n' "$desc" "$expected" "$actual" + ((failed++)) || true + fi +} + +assert_contains() { + local desc="$1" needle="$2" haystack="$3" + if [[ "$haystack" == *"$needle"* ]]; then + printf '\033[1;32mPASS\033[0m %s\n' "$desc" + ((passed++)) || true + else + printf '\033[1;31mFAIL\033[0m %s\n expected to contain: %s\n actual: %s\n' "$desc" "$needle" "$haystack" + ((failed++)) || true + fi +} + +assert_not_contains() { + local desc="$1" needle="$2" haystack="$3" + if [[ "$haystack" != *"$needle"* ]]; then + printf '\033[1;32mPASS\033[0m %s\n' "$desc" + ((passed++)) || true + else + printf '\033[1;31mFAIL\033[0m %s\n expected NOT to contain: %s\n' "$desc" "$needle" + ((failed++)) || true + fi +} + +# --- remove_existing_gh_token tests --- + +test_remove_existing_gh_token_removes_line() { + local tmpfile + tmpfile=$(mktemp) + cat > "$tmpfile" <<'EOF' +# some config +export PATH="/usr/bin:$PATH" +export GH_TOKEN='ghp_abc123' +export OTHER_VAR="hello" +EOF + remove_existing_gh_token "$tmpfile" + local content + content=$(cat "$tmpfile") + assert_not_contains "remove_existing: GH_TOKEN line removed" "export GH_TOKEN=" "$content" + assert_contains "remove_existing: PATH line preserved" "export PATH=" "$content" + assert_contains "remove_existing: OTHER_VAR preserved" "export OTHER_VAR=" "$content" + rm -f "$tmpfile" +} + +test_remove_existing_gh_token_no_match() { + local tmpfile + tmpfile=$(mktemp) + cat > "$tmpfile" <<'EOF' +export PATH="/usr/bin:$PATH" +export OTHER_VAR="hello" +EOF + local before after + before=$(cat "$tmpfile") + remove_existing_gh_token "$tmpfile" + after=$(cat "$tmpfile") + assert_eq "remove_existing: no GH_TOKEN, file unchanged" "$before" "$after" + rm -f "$tmpfile" +} + +test_remove_existing_no_bak_file() { + local tmpfile + tmpfile=$(mktemp) + echo 'export GH_TOKEN="old"' > "$tmpfile" + remove_existing_gh_token "$tmpfile" + local bak_exists="no" + [[ -f "${tmpfile}.bak" ]] && bak_exists="yes" + assert_eq "remove_existing: no .bak file left" "no" "$bak_exists" + rm -f "$tmpfile" +} + +# --- store_token tests (Linux path) --- + +test_store_token_linux_writes_export() { + # Force Linux path by overriding uname + # shellcheck disable=SC2329 + uname() { echo "Linux"; } + export -f uname + + local tmpfile + tmpfile=$(mktemp) + echo '# existing config' > "$tmpfile" + store_token "ghp_test123" "$tmpfile" + local content + content=$(cat "$tmpfile") + assert_contains "store_token linux: writes export" "export GH_TOKEN='ghp_test123'" "$content" + assert_contains "store_token linux: preserves existing" "# existing config" "$content" + + unset -f uname + rm -f "$tmpfile" +} + +test_store_token_replaces_existing() { + # shellcheck disable=SC2329 + uname() { echo "Linux"; } + export -f uname + + local tmpfile + tmpfile=$(mktemp) + cat > "$tmpfile" <<'EOF' +# config +export GH_TOKEN='old_token' +export OTHER="val" +EOF + store_token "ghp_new" "$tmpfile" + local content + content=$(cat "$tmpfile") + assert_not_contains "store_token replace: old token removed" "old_token" "$content" + assert_contains "store_token replace: new token present" "export GH_TOKEN='ghp_new'" "$content" + assert_contains "store_token replace: other vars preserved" "export OTHER=" "$content" + + unset -f uname + rm -f "$tmpfile" +} + +# --- Run all tests --- + +echo "=== remove_existing_gh_token ===" +test_remove_existing_gh_token_removes_line +test_remove_existing_gh_token_no_match +test_remove_existing_no_bak_file + +echo "" +echo "=== store_token (Linux path) ===" +test_store_token_linux_writes_export +test_store_token_replaces_existing + +echo "" +echo "---" +echo "Passed: $passed Failed: $failed" +[[ "$failed" -eq 0 ]] From 9874c14f954ffb76184eb1e634f5cb2433a9388a Mon Sep 17 00:00:00 2001 From: Raul Date: Fri, 13 Mar 2026 23:34:05 +0100 Subject: [PATCH 3/4] Address second round of PR #6 review comments - Log stderr from gh auth status on failure for debuggability - Replace grep/head/cut chain with jq in curl fallback for robust JSON parsing in validate_token() - chmod 600 shell profile after writing plaintext token on Linux - Add test for stderr logging and chmod 600 behavior Co-Authored-By: Claude Opus 4.6 --- .devcontainer/post_install.py | 7 +++++++ scripts/setup-gh-token.sh | 5 +++-- tests/test_post_install.py | 20 +++++++++++++++++--- tests/test_setup_gh_token.sh | 18 ++++++++++++++++++ 4 files changed, 45 insertions(+), 5 deletions(-) diff --git a/.devcontainer/post_install.py b/.devcontainer/post_install.py index 473d94e..e7f00f8 100644 --- a/.devcontainer/post_install.py +++ b/.devcontainer/post_install.py @@ -441,6 +441,7 @@ def setup_gh_auth(): result = subprocess.run( ["gh", "auth", "status"], capture_output=True, + text=True, ) if result.returncode != 0: print( @@ -449,6 +450,12 @@ def setup_gh_auth(): "run 'gh auth login' inside the container.", file=sys.stderr, ) + if result.stderr.strip(): + print( + f"[post_install] gh auth status: " + f"{result.stderr.strip()}", + file=sys.stderr, + ) return # Check token type from gh auth token_result = subprocess.run( diff --git a/scripts/setup-gh-token.sh b/scripts/setup-gh-token.sh index 63a7f40..5cd919c 100755 --- a/scripts/setup-gh-token.sh +++ b/scripts/setup-gh-token.sh @@ -29,7 +29,7 @@ validate_token() { -H "Authorization: Bearer ${token}" \ -H "Accept: application/vnd.github+json" \ "https://api.github.com/user" 2>/dev/null | \ - grep -o '"login":"[^"]*"' | head -1 | cut -d'"' -f4 + jq -r '.login // empty' fi } @@ -102,8 +102,9 @@ store_token() { printf '\n# GitHub PAT for Claude Code (gh CLI + git HTTPS)\nexport GH_TOKEN=$(security find-generic-password -a "$USER" -s "claude-code-gh-token" -w 2>/dev/null)\n' >> "$profile" info "Token stored in macOS Keychain" else - warn "No secure credential store found. Token will be stored in plaintext in $profile." + warn "No secure credential store found. Token will be stored in plaintext in $profile (mode 600)." printf '\n# GitHub PAT for Claude Code (gh CLI + git HTTPS)\nexport GH_TOKEN='\''%s'\''\n' "$token" >> "$profile" + chmod 600 "$profile" fi } diff --git a/tests/test_post_install.py b/tests/test_post_install.py index 030dbc9..f6c81f8 100644 --- a/tests/test_post_install.py +++ b/tests/test_post_install.py @@ -70,17 +70,31 @@ def test_invalid_gh_token_returns_early(self, mock_run, capsys): @patch("post_install.subprocess.run") @patch.dict("os.environ", {}, clear=False) def test_no_token_no_gh_auth(self, mock_run, capsys): - # Remove GH_TOKEN if present import os os.environ.pop("GH_TOKEN", None) - mock_run.return_value = MagicMock(returncode=1) + mock_run.return_value = MagicMock(returncode=1, stderr="", stdout="") post_install.setup_gh_auth() err = capsys.readouterr().err assert "No GitHub auth found" in err - # Only gh auth status called, no setup-git assert mock_run.call_count == 1 + @patch("post_install.subprocess.run") + @patch.dict("os.environ", {}, clear=False) + def test_no_token_no_gh_auth_logs_stderr(self, mock_run, capsys): + import os + os.environ.pop("GH_TOKEN", None) + + mock_run.return_value = MagicMock( + returncode=1, + stderr="You are not logged into any GitHub hosts.", + stdout="", + ) + post_install.setup_gh_auth() + err = capsys.readouterr().err + assert "No GitHub auth found" in err + assert "not logged into" in err + @patch("post_install.subprocess.run") @patch.dict("os.environ", {}, clear=False) def test_no_token_gh_auth_succeeds(self, mock_run, capsys): diff --git a/tests/test_setup_gh_token.sh b/tests/test_setup_gh_token.sh index 0dbfc06..3a8d86a 100644 --- a/tests/test_setup_gh_token.sh +++ b/tests/test_setup_gh_token.sh @@ -137,6 +137,23 @@ EOF rm -f "$tmpfile" } +test_store_token_linux_chmod_600() { + # shellcheck disable=SC2329 + uname() { echo "Linux"; } + export -f uname + + local tmpfile + tmpfile=$(mktemp) + chmod 644 "$tmpfile" + store_token "ghp_test" "$tmpfile" + local perms + perms=$(stat -f '%Lp' "$tmpfile" 2>/dev/null || stat -c '%a' "$tmpfile" 2>/dev/null) + assert_eq "store_token linux: file mode 600" "600" "$perms" + + unset -f uname + rm -f "$tmpfile" +} + # --- Run all tests --- echo "=== remove_existing_gh_token ===" @@ -148,6 +165,7 @@ echo "" echo "=== store_token (Linux path) ===" test_store_token_linux_writes_export test_store_token_replaces_existing +test_store_token_linux_chmod_600 echo "" echo "---" From 4f83a34378121f1394f458f59f8b7ab3c10af676 Mon Sep 17 00:00:00 2001 From: Raul Date: Mon, 16 Mar 2026 19:12:35 +0100 Subject: [PATCH 4/4] Add mv error handling in remove_existing_gh_token If mv fails, clean up the tmpfile and return non-zero instead of leaving a dangling temp file and continuing with a missing profile. Co-Authored-By: Claude Opus 4.6 --- scripts/setup-gh-token.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/setup-gh-token.sh b/scripts/setup-gh-token.sh index 5cd919c..e1c2907 100755 --- a/scripts/setup-gh-token.sh +++ b/scripts/setup-gh-token.sh @@ -81,7 +81,7 @@ remove_existing_gh_token() { local tmpfile tmpfile=$(mktemp "${profile}.XXXXXX") grep -v "^export GH_TOKEN=" "$profile" > "$tmpfile" || true - mv "$tmpfile" "$profile" + mv "$tmpfile" "$profile" || { rm -f "$tmpfile"; return 1; } fi }