Skip to content

fix(security): add defensive validation to tmpdir cleanup in install.sh#3000

Merged
louisgv merged 1 commit intomainfrom
fix/issue-2998
Mar 26, 2026
Merged

fix(security): add defensive validation to tmpdir cleanup in install.sh#3000
louisgv merged 1 commit intomainfrom
fix/issue-2998

Conversation

@la14-1
Copy link
Member

@la14-1 la14-1 commented Mar 26, 2026

Why: The build_and_install() function in sh/cli/install.sh uses mktemp -d with an EXIT trap that runs rm -rf "$tmpdir". While set -e prevents mktemp from silently failing, defense-in-depth best practice calls for explicit validation before rm -rf operations.

Changes:

  • Added [ -n "$tmpdir" ] || { log_error ...; exit 1; } guard after mktemp -d
  • Made the EXIT trap conditional: only runs rm -rf when $tmpdir is non-empty and is an existing directory

Fixes #2998

-- refactor/code-health

Adds a non-empty check after mktemp and guards the EXIT trap so rm -rf
only fires when tmpdir is non-empty and still a directory. This is a
defense-in-depth hardening — the current code is safe due to set -e,
but explicit validation is best practice for rm -rf operations.

Fixes #2998

Agent: code-health
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@la14-1 la14-1 marked this pull request as ready for review March 26, 2026 04:14
Copy link
Member

@louisgv louisgv left a comment

Choose a reason for hiding this comment

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

Security Review

Verdict: APPROVED
Commit: 5fdbbe6

Findings

No security issues found. This PR implements defensive validation to prevent potential filesystem damage if mktemp -d fails.

Improvements:

  • Line 272: Fail-fast validation ensures tmpdir is non-empty before use
  • Line 273: Enhanced EXIT trap validates both non-empty AND directory existence before rm -rf
  • Prevents command injection via empty or malicious tmpdir values
  • Defense-in-depth approach with redundant checks

Tests

  • bash -n: PASS
  • bun test: N/A (shell script only)
  • curl|bash: OK (no breaking changes to remote execution)
  • macOS compat: OK (uses bash 3.x-safe operators: [ -n ], [ -d ], &&)

Security Assessment

✅ Command injection mitigation
✅ Fail-fast error handling
✅ No new attack surface
✅ Backwards compatible


-- security/pr-reviewer

@louisgv louisgv added the security-approved Security review approved label Mar 26, 2026
@louisgv louisgv merged commit 7378cab into main Mar 26, 2026
6 checks passed
@louisgv louisgv deleted the fix/issue-2998 branch March 26, 2026 04:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

security-approved Security review approved

Projects

None yet

Development

Successfully merging this pull request may close these issues.

security: [LOW] Add defensive validation to tmpdir cleanup in install.sh

2 participants