feat: comprehensive codebase refinement for security, performance, and robustness#10
Conversation
…d robustness - fix(security): enforce 700/600 permissions on secrets.d and backups - fix(security): enforce umask 077 on ssh agent socket and local secrets file - fix(security): escape identity variables to prevent get-bashedrc injection - fix(security): resolve true path in get_bashed_component instead of trusting env var - perf(startup): eliminate multiple brew --prefix subshells to accelerate bashrc load - fix(logic): correct HISTIGNORE typo that neutralized the ignore list - chore: tighten bin/gen_tool_versions with strict bash flags
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 20 minutes and 54 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughRefactors Homebrew prefix discovery to compute a single BREW_PREFIX and export Homebrew env vars directly; replaces eval-based invocations. Adds globstar, adjusts HISTIGNORE, tightens umask/permissions for secrets and ssh-agent, hardens scripts with strict mode and traps, adds mkcd and eza support, and updates tests for quoting and installer flags. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR delivers solid security and performance improvements to the get-bashed environment. The changes eliminate expensive brew subshells, properly secure sensitive directories, escape user input, and improve error handling.
Critical Issue Found (1)
bashrc.d/30-buildflags.sh: The new path construction approach doesn't verify that optional package directories exist before adding them to build flags. This will cause build failures when packages like readline, gettext, or zstd aren't installed. The old brew --prefix approach handled missing packages gracefully by returning empty strings.
Changes Verified
The following improvements are correctly implemented:
- Performance optimization replacing multiple
brew --prefixcalls with single calculation - Security hardening with
umask 077for SSH agent socket and secrets file - Security enforcement with
chmod 700on backup and secrets directories - Input sanitization escaping quotes in user-provided name/email fields
- Path traversal prevention using script's physical location
- HISTIGNORE pattern fix (adding missing colon after
ls *) - Error handling improvements with
set -euo pipefailand proper null checks - Test coverage for quote escaping functionality
Please address the critical issue in 30-buildflags.sh before merging.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements performance optimizations by replacing Homebrew subshell calls with manual environment variable exports and improves security by enforcing stricter file permissions and umasks. It also enhances script reliability through better error handling and temporary file cleanup. Review feedback identifies a potential command injection vulnerability in the manual string escaping logic and a path inaccuracy for Homebrew repositories on Intel-based Macs.
There was a problem hiding this comment.
Pull request overview
This PR refines the get-bashed shell environment and installer with a focus on security hardening, faster initialization, and improved robustness across scripts and tests.
Changes:
- Tighten permissions/umask around secrets, backups, and ssh-agent socket creation.
- Reduce Homebrew-related startup overhead by avoiding repeated
brew --prefixcalls and using a computed prefix path. - Improve installer/test robustness (non-interactive
--auto, better cleanup traps, saferasdfhandling) and fixHISTIGNORE.
Reviewed changes
Copilot reviewed 10 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/install.bats | Runs installer in non-interactive mode (--auto) during tests. |
| tests/config_output.bats | Adds test coverage for escaping quotes in generated identity exports. |
| scripts/verify-install.sh | Ensures temp dirs are cleaned up via trap on exit. |
| scripts/gen-docs.sh | Adds cleanup traps for temp files used during docs generation. |
| install.bash | Hardens permissions for secrets/backup dir and escapes identity values in generated config. |
| bin/get_bashed_component | Derives GET_BASHED_HOME from script location instead of env/default. |
| bin/gen_tool_versions | Adds set -euo pipefail and makes asdf interactions tolerant of empty/missing state. |
| bashrc.d/95-ssh-agent.sh | Attempts to apply umask 077 when starting ssh-agent. |
| bashrc.d/30-buildflags.sh | Replaces repeated brew --prefix calls with a computed Homebrew prefix path. |
| bashrc.d/20-path.sh | Similar Homebrew prefix optimization for GNU tool path injection. |
| bashrc.d/00-options.sh | Fixes HISTIGNORE so ps commands are properly ignored. |
| bash_profile | Replaces brew shellenv with manual environment variable exports for faster login. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
bin/gen_tool_versions (1)
7-14: Consider snapshotting the plugin list once.Lines 7 and 12 both call
asdf plugin list; storing it in a variable removes the duplicate external command call and ensures both loops operate on the same snapshot.♻️ Proposed refactor
-for plugin in $(asdf plugin list 2>/dev/null || true); do +plugins="$(asdf plugin list 2>/dev/null || true)" + +for plugin in $plugins; do [[ -z "$plugin" ]] && continue asdf install "$plugin" latest done -for plugin in $(asdf plugin list 2>/dev/null || true); do +for plugin in $plugins; do [[ -z "$plugin" ]] && continue versions=$(asdf list "$plugin" 2>/dev/null | sed 's/^[[:space:]]*//' | grep -v '^latest$' || true)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/gen_tool_versions` around lines 7 - 14, Capture the output of the external command asdf plugin list once into a variable (e.g., plugin_list) and use that variable in both for loops instead of calling asdf plugin list twice; update the loops that currently iterate over $(asdf plugin list 2>/dev/null || true) to iterate over the saved plugin_list, preserving the existing empty-plugin guard ([[ -z "$plugin" ]] && continue) and subsequent logic that computes versions=$(asdf list "$plugin" ...), so both loops operate on the same snapshot and avoid duplicated external calls.scripts/gen-docs.sh (1)
66-79: Minor cleanup: choose one temp-file cleanup path forTMP_MODULES.Line 66 already guarantees removal on exit; the explicit
rm -f "$TMP_MODULES"at Line 79 is redundant. Keeping a single cleanup path makes this easier to maintain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/gen-docs.sh` around lines 66 - 79, The TMP_MODULES temporary file is already removed via the EXIT trap (trap 'rm -f "$TMP_MODULES"' EXIT), so remove the redundant explicit rm -f "$TMP_MODULES" at the end of the script to keep a single cleanup path; ensure the trap and any early exits still cover TMP_MODULES lifecycle and leave the shdoc < "$TMP_MODULES" invocation and surrounding loop (which populates TMP_MODULES) unchanged.install.bash (1)
565-570: Quote escaping prevents command injection—consider additional characters.The
${USER_NAME//\"/\\\"}pattern correctly escapes double quotes. However, other characters could also cause issues when the generated file is sourced:
- Backticks (
`) enable command substitution$enables variable/command expansion- Backslashes (
\) can escape subsequent charactersFor defense-in-depth, consider escaping backslashes and dollar signs as well, or validating input more strictly.
♻️ More comprehensive escaping (optional)
if [[ -n "$USER_NAME" ]]; then - echo "export GET_BASHED_USER_NAME=\"${USER_NAME//\"/\\\"}\"" + _escaped="${USER_NAME//\\/\\\\}" # escape backslashes first + _escaped="${_escaped//\$/\\\$}" # escape dollar signs + _escaped="${_escaped//\"/\\\"}" # escape double quotes + _escaped="${_escaped//\`/\\\`}" # escape backticks + echo "export GET_BASHED_USER_NAME=\"${_escaped}\"" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.bash` around lines 565 - 570, The export lines for USER_NAME and USER_EMAIL only escape double quotes; update the generation to also escape backslashes and dollar signs (and ideally backticks) so the produced export statements are safe when sourced: first escape backslashes, then escape double quotes and dollar signs (or validate/whitelist USER_NAME/USER_EMAIL) before echoing the export lines. Target the two export-generation sites that reference USER_NAME and USER_EMAIL and apply the expanded escaping/validation there.bashrc.d/30-buildflags.sh (1)
17-22: Consider gating on formula existence to avoid build errors.If
readline,gettext, orzstdaren't installed, the exported paths will reference non-existent directories, potentially causing confusing build failures. The OpenSSL existence check pattern could be extended to other formulas.♻️ Optional: skip missing formulas
BREW_PREFIX="$(dirname "$(dirname "$(command -v brew)")")" OPENSSL_PREFIX="$BREW_PREFIX/opt/openssl@3" [[ -d "$OPENSSL_PREFIX" ]] || OPENSSL_PREFIX="$BREW_PREFIX/opt/openssl" - READLINE_PREFIX="$BREW_PREFIX/opt/readline" - GETTEXT_PREFIX="$BREW_PREFIX/opt/gettext" - ZSTD_PREFIX="$BREW_PREFIX/opt/zstd" + READLINE_PREFIX="$BREW_PREFIX/opt/readline" + GETTEXT_PREFIX="$BREW_PREFIX/opt/gettext" + ZSTD_PREFIX="$BREW_PREFIX/opt/zstd" - export LDFLAGS="-L${OPENSSL_PREFIX}/lib -L${READLINE_PREFIX}/lib -L${GETTEXT_PREFIX}/lib -L${ZSTD_PREFIX}/lib ${LDFLAGS}" + _ldflags="" + [[ -d "$OPENSSL_PREFIX/lib" ]] && _ldflags+="-L${OPENSSL_PREFIX}/lib " + [[ -d "$READLINE_PREFIX/lib" ]] && _ldflags+="-L${READLINE_PREFIX}/lib " + [[ -d "$GETTEXT_PREFIX/lib" ]] && _ldflags+="-L${GETTEXT_PREFIX}/lib " + [[ -d "$ZSTD_PREFIX/lib" ]] && _ldflags+="-L${ZSTD_PREFIX}/lib " + export LDFLAGS="${_ldflags}${LDFLAGS}"This is optional since the original code also referenced potentially missing directories.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bashrc.d/30-buildflags.sh` around lines 17 - 22, Update the exports (LDFLAGS, CPPFLAGS, PKG_CONFIG_PATH, LIBRARY_PATH, CPATH, PYTHON_CONFIGURE_OPTS) to only append paths for formulas that actually exist by checking each prefix directory (OPENSSL_PREFIX, READLINE_PREFIX, GETTEXT_PREFIX, ZSTD_PREFIX) before referencing it; follow the same existence-check pattern used for OpenSSL (e.g., test -d "${OPENSSL_PREFIX}/lib") and conditionally concatenate the corresponding "-L", "-I", pkgconfig, or include entries so missing formulas aren’t exported into the environment and cause build errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bashrc.d/95-ssh-agent.sh`:
- Line 24: The subshell form (umask 077; eval "$(ssh-agent -a "$SSH_AGENT_SOCK"
-s)" >/dev/null) discards SSH_AUTH_SOCK/SSH_AGENT_PID; change it so ssh-agent is
started in the current shell and the evaluated assignments are exported to the
parent: run umask 077, capture the output of eval "$(ssh-agent -a
"$SSH_AGENT_SOCK" -s)" into the current shell (so SSH_AUTH_SOCK/SSH_AGENT_PID
are set), export SSH_AUTH_SOCK (and SSH_AGENT_PID if needed), then restore the
previous umask before calling ssh-add so subsequent ssh-add commands can see the
socket.
In `@bin/get_bashed_component`:
- Around line 35-37: When resolving GET_BASHED_HOME from the script path, the
current assignment using SCRIPT_DIR and dirname fails for symlinked invocations;
update the resolution to canonicalize the script location by following symlinks
(e.g., resolve real_script by iterating on readlink/realpath of
"${BASH_SOURCE[0]}" until not a symlink) and then set SCRIPT_DIR from the
canonical real_script's dirname, and finally derive GET_BASHED_HOME from that
SCRIPT_DIR; change references around SCRIPT_DIR and GET_BASHED_HOME to use the
resolved canonical path so sourced helpers are found even when the CLI is
invoked via a symlink.
---
Nitpick comments:
In `@bashrc.d/30-buildflags.sh`:
- Around line 17-22: Update the exports (LDFLAGS, CPPFLAGS, PKG_CONFIG_PATH,
LIBRARY_PATH, CPATH, PYTHON_CONFIGURE_OPTS) to only append paths for formulas
that actually exist by checking each prefix directory (OPENSSL_PREFIX,
READLINE_PREFIX, GETTEXT_PREFIX, ZSTD_PREFIX) before referencing it; follow the
same existence-check pattern used for OpenSSL (e.g., test -d
"${OPENSSL_PREFIX}/lib") and conditionally concatenate the corresponding "-L",
"-I", pkgconfig, or include entries so missing formulas aren’t exported into the
environment and cause build errors.
In `@bin/gen_tool_versions`:
- Around line 7-14: Capture the output of the external command asdf plugin list
once into a variable (e.g., plugin_list) and use that variable in both for loops
instead of calling asdf plugin list twice; update the loops that currently
iterate over $(asdf plugin list 2>/dev/null || true) to iterate over the saved
plugin_list, preserving the existing empty-plugin guard ([[ -z "$plugin" ]] &&
continue) and subsequent logic that computes versions=$(asdf list "$plugin"
...), so both loops operate on the same snapshot and avoid duplicated external
calls.
In `@install.bash`:
- Around line 565-570: The export lines for USER_NAME and USER_EMAIL only escape
double quotes; update the generation to also escape backslashes and dollar signs
(and ideally backticks) so the produced export statements are safe when sourced:
first escape backslashes, then escape double quotes and dollar signs (or
validate/whitelist USER_NAME/USER_EMAIL) before echoing the export lines. Target
the two export-generation sites that reference USER_NAME and USER_EMAIL and
apply the expanded escaping/validation there.
In `@scripts/gen-docs.sh`:
- Around line 66-79: The TMP_MODULES temporary file is already removed via the
EXIT trap (trap 'rm -f "$TMP_MODULES"' EXIT), so remove the redundant explicit
rm -f "$TMP_MODULES" at the end of the script to keep a single cleanup path;
ensure the trap and any early exits still cover TMP_MODULES lifecycle and leave
the shdoc < "$TMP_MODULES" invocation and surrounding loop (which populates
TMP_MODULES) unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff955169-7f6c-4b3a-83bb-66c736e73699
📒 Files selected for processing (12)
bash_profilebashrc.d/00-options.shbashrc.d/20-path.shbashrc.d/30-buildflags.shbashrc.d/95-ssh-agent.shbin/gen_tool_versionsbin/get_bashed_componentinstall.bashscripts/gen-docs.shscripts/verify-install.shtests/config_output.batstests/install.bats
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
bashrc.d/90-functions.sh (1)
31-33: Hardenmkcdargument handling for empty args and dash-prefixed paths.At Line 32, missing-arg calls fail with an unclear error, and dash-prefixed directory names can be parsed as options. Add explicit arg validation and
--terminators.Proposed patch
mkcd() { - mkdir -p "$1" && cd "$1" || return 1 + [[ $# -eq 1 && -n "$1" ]] || { echo "Usage: mkcd <dir>" >&2; return 2; } + mkdir -p -- "$1" && cd -- "$1" || return 1 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bashrc.d/90-functions.sh` around lines 31 - 33, The mkcd function should explicitly validate its argument and protect against dash-prefixed names: check that the first parameter is provided and non-empty (e.g., test [ -z "${1-}" ] and return with an error/exit status if missing), then call mkdir and cd with a -- terminator to avoid treating names starting with - as options (use mkdir -p -- "$1" && cd -- "$1" || return 1). Update the mkcd function body accordingly to perform the arg check and use -- with mkdir and cd.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@install.bash`:
- Around line 566-570: The export lines for GET_BASHED_USER_NAME and
GET_BASHED_USER_EMAIL are vulnerable because they only escape double quotes and
still allow $( ), backticks, or $VAR expansions when the generated assignments
are sourced; update the echo logic that writes these exports (the lines
referencing USER_NAME and USER_EMAIL) to produce a safe literal string: either
use a shell-safe quoting method (e.g., single-quote the value and escape
embedded single quotes) or emit a properly escaped/quoted form via printf '%q'
so no command substitution or variable expansion can occur when sourcing the
config; ensure both GET_BASHED_USER_NAME and GET_BASHED_USER_EMAIL use the same
safe approach.
- Around line 543-552: The script only sets strict permissions when creating
"$PREFIX/secrets.d/00-local.sh", so reruns won’t fix an existing file with
permissive modes; after the creation block always normalize permissions by
running chmod 600 "$PREFIX/secrets.d/00-local.sh" (leave the umask 077 + heredoc
creation as-is to create safely when missing, then unconditionally enforce the
file mode on every run).
---
Nitpick comments:
In `@bashrc.d/90-functions.sh`:
- Around line 31-33: The mkcd function should explicitly validate its argument
and protect against dash-prefixed names: check that the first parameter is
provided and non-empty (e.g., test [ -z "${1-}" ] and return with an error/exit
status if missing), then call mkdir and cd with a -- terminator to avoid
treating names starting with - as options (use mkdir -p -- "$1" && cd -- "$1" ||
return 1). Update the mkcd function body accordingly to perform the arg check
and use -- with mkdir and cd.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 75cd77ca-1cba-4d30-a6fc-3a6d006b59c0
📒 Files selected for processing (6)
bash_aliasesbashrc.d/00-options.shbashrc.d/90-functions.shdocs/index.mdinstall.bashinstallers/tools.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- bashrc.d/00-options.sh
| chmod 700 "$PREFIX/secrets.d" | ||
| if [[ ! -e "$PREFIX/secrets.d/00-local.sh" ]]; then | ||
| cat <<'__SECRETS__' > "$PREFIX/secrets.d/00-local.sh" | ||
| ( | ||
| umask 077 | ||
| cat <<'__SECRETS__' > "$PREFIX/secrets.d/00-local.sh" | ||
| # Place local secrets here. Example: | ||
| # export FOO="bar" | ||
| __SECRETS__ | ||
| ) | ||
| fi |
There was a problem hiding this comment.
Normalize 00-local.sh mode on every run, not only on creation.
Right now, Line 544 only creates the file if missing. If an existing 00-local.sh has permissive mode from an older install, reruns won’t correct it.
🔧 Proposed fix
mkdir -p "$PREFIX/secrets.d"
chmod 700 "$PREFIX/secrets.d"
if [[ ! -e "$PREFIX/secrets.d/00-local.sh" ]]; then
(
umask 077
cat <<'__SECRETS__' > "$PREFIX/secrets.d/00-local.sh"
# Place local secrets here. Example:
# export FOO="bar"
__SECRETS__
)
fi
+chmod 600 "$PREFIX/secrets.d/00-local.sh"As per coding guidelines: Every script and module must be idempotent and safe to re-run without causing side effects or errors.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| chmod 700 "$PREFIX/secrets.d" | |
| if [[ ! -e "$PREFIX/secrets.d/00-local.sh" ]]; then | |
| cat <<'__SECRETS__' > "$PREFIX/secrets.d/00-local.sh" | |
| ( | |
| umask 077 | |
| cat <<'__SECRETS__' > "$PREFIX/secrets.d/00-local.sh" | |
| # Place local secrets here. Example: | |
| # export FOO="bar" | |
| __SECRETS__ | |
| ) | |
| fi | |
| chmod 700 "$PREFIX/secrets.d" | |
| if [[ ! -e "$PREFIX/secrets.d/00-local.sh" ]]; then | |
| ( | |
| umask 077 | |
| cat <<'__SECRETS__' > "$PREFIX/secrets.d/00-local.sh" | |
| # Place local secrets here. Example: | |
| # export FOO="bar" | |
| __SECRETS__ | |
| ) | |
| fi | |
| chmod 600 "$PREFIX/secrets.d/00-local.sh" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@install.bash` around lines 543 - 552, The script only sets strict permissions
when creating "$PREFIX/secrets.d/00-local.sh", so reruns won’t fix an existing
file with permissive modes; after the creation block always normalize
permissions by running chmod 600 "$PREFIX/secrets.d/00-local.sh" (leave the
umask 077 + heredoc creation as-is to create safely when missing, then
unconditionally enforce the file mode on every run).
|
700/600permissions onsecrets.dand backups to prevent exposing local secrets.umask 077when generating the SSH Agent socket and the00-local.shstub to prevent standard755leakage.$GET_BASHED_HOMEexecution inbin/get_bashed_component, utilizing the physical location of the script.--nameand--emailstrings to prevent Bash injection attacks insideget-bashedrc.sh.$(brew --prefix pkg)subshells during login and load. These have been aggressively combined into a single relative-path calculation string (viaBREW_PREFIX), shaving 50ms+ off initialization.HISTIGNOREstring that failed to filterpscommands.set -euo pipefailtobin/gen_tool_versionsand handled empty ASDF lists gracefully. Added background execution capability to BATS testing.These macro and micro corrections solidify the base environment's posture.
Summary by CodeRabbit
New Features
ezalisting tool and conditional aliases to use it when available.Bug Fixes
Chores
Tests