diff --git a/CHANGELOG.md b/CHANGELOG.md index 5c06108..db66098 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,8 @@ and versions are tracked in the repo-root `VERSION` file. ### Fixed +- Hardened named-output helpers across std, string, arg, git, and GitHub + libraries against caller variable names that collide with helper internals. - Made `gh_run` report failed GitHub CLI commands even when callers enable `set -e`, and preserved argument boundaries in GitHub command failure logs. - Documented and tested `str_split` trailing-separator behavior. diff --git a/lib/bash/arg/lib_arg.sh b/lib/bash/arg/lib_arg.sh index d15847e..5c6b896 100644 --- a/lib/bash/arg/lib_arg.sh +++ b/lib/bash/arg/lib_arg.sh @@ -11,21 +11,21 @@ fi readonly __lib_arg_sourced__=1 __arg_set_assoc_value__() { - local array_name="$1" key="$2" value="$3" + local __arg_assoc_name="$1" __arg_assoc_key="$2" __arg_assoc_value="$3" # The variable name is validated before callers reach this helper. # shellcheck disable=SC1087 - printf -v "$array_name[$key]" '%s' "$value" + printf -v "$__arg_assoc_name[$__arg_assoc_key]" '%s' "$__arg_assoc_value" } __arg_parse_specs__() { - local specs_name="$1" + local __arg_specs_name="$1" local __arg_token_kind_name="$2" __arg_token_name_name="$3" local -a __arg_specs=() __arg_tokens=() local __arg_spec __arg_remainder __arg_name __arg_kind __arg_tokens_part __arg_token local __arg_name_re='^[A-Za-z_][A-Za-z0-9_]*$' - eval "__arg_specs=(\"\${${specs_name}[@]}\")" + eval "__arg_specs=(\"\${${__arg_specs_name}[@]}\")" for __arg_spec in "${__arg_specs[@]}"; do __arg_name="${__arg_spec%%|*}" @@ -76,7 +76,7 @@ __arg_parse_specs__() { # arg_parse options positionals specs -- "$@" # arg_parse() { - local options_name="${1-}" positionals_name="${2-}" specs_name="${3-}" + local __arg_options_name="${1-}" __arg_positionals_name="${2-}" __arg_specs_name="${3-}" local __arg_current __arg_option_token __arg_option_value __arg_option_name __arg_option_kind local -a __arg_positionals=() local -A __arg_token_kind=() __arg_token_name=() @@ -87,25 +87,25 @@ arg_parse() { return 2 fi - assert_variable_name "$options_name" "$positionals_name" "$specs_name" + assert_variable_name "$__arg_options_name" "$__arg_positionals_name" "$__arg_specs_name" - if ! __std_declares_array_kind__ "$options_name" "A"; then + if ! __std_declares_array_kind__ "$__arg_options_name" "A"; then log_error "arg_parse: options variable must be an associative array declared by the caller." return 2 fi - if ! __std_declares_array_kind__ "$positionals_name" "a"; then + if ! __std_declares_array_kind__ "$__arg_positionals_name" "a"; then log_error "arg_parse: positionals variable must be an indexed array declared by the caller." return 2 fi - if ! __std_declares_array_kind__ "$specs_name" "a"; then + if ! __std_declares_array_kind__ "$__arg_specs_name" "a"; then log_error "arg_parse: specs variable must be an indexed array declared by the caller." return 2 fi - __arg_parse_specs__ "$specs_name" __arg_token_kind __arg_token_name || return $? + __arg_parse_specs__ "$__arg_specs_name" __arg_token_kind __arg_token_name || return $? - eval "$options_name=()" - eval "$positionals_name=()" + eval "$__arg_options_name=()" + eval "$__arg_positionals_name=()" shift 4 while (($# > 0)); do @@ -132,7 +132,7 @@ arg_parse() { return 2 fi - __arg_set_assoc_value__ "$options_name" "$__arg_option_name" "$__arg_option_value" + __arg_set_assoc_value__ "$__arg_options_name" "$__arg_option_name" "$__arg_option_value" continue fi @@ -147,7 +147,7 @@ arg_parse() { fi if [[ "$__arg_option_kind" == "flag" ]]; then - __arg_set_assoc_value__ "$options_name" "$__arg_option_name" "1" + __arg_set_assoc_value__ "$__arg_options_name" "$__arg_option_name" "1" continue fi @@ -162,13 +162,13 @@ arg_parse() { __arg_option_value="$1" shift - __arg_set_assoc_value__ "$options_name" "$__arg_option_name" "$__arg_option_value" + __arg_set_assoc_value__ "$__arg_options_name" "$__arg_option_name" "$__arg_option_value" continue fi __arg_positionals+=("$__arg_current") done - eval "$positionals_name=(\"\${__arg_positionals[@]}\")" + eval "$__arg_positionals_name=(\"\${__arg_positionals[@]}\")" return 0 } diff --git a/lib/bash/arg/tests/lib_arg.bats b/lib/bash/arg/tests/lib_arg.bats index 5657bf1..62aaa18 100644 --- a/lib/bash/arg/tests/lib_arg.bats +++ b/lib/bash/arg/tests/lib_arg.bats @@ -55,6 +55,22 @@ create_script() { [ "${positionals[2]}" = "gamma" ] } +@test "arg_parse supports shadowing-prone caller array names" { + local -a specs_name=( + "verbose|flag|--verbose|-v" + "output|value|--output|-o" + ) + local -A options_name=() + local -a positionals_name=() + + arg_parse options_name positionals_name specs_name -- -v --output result.txt item + + [ "${options_name[verbose]}" = "1" ] + [ "${options_name[output]}" = "result.txt" ] + [ "${#positionals_name[@]}" -eq 1 ] + [ "${positionals_name[0]}" = "item" ] +} + @test "arg_parse accepts long option equals values and repeated options" { local -a specs=( "verbose|flag|--verbose|-v" diff --git a/lib/bash/gh/lib_gh.sh b/lib/bash/gh/lib_gh.sh index 644ac65..e74f323 100644 --- a/lib/bash/gh/lib_gh.sh +++ b/lib/bash/gh/lib_gh.sh @@ -61,38 +61,38 @@ gh_run() { } gh_repo_from_remote_url() { - local remote_url="$1" - local result_var="${2:-}" - local parsed_repo + local __gh_remote_url="$1" + local __gh_result_name="${2:-}" + local __gh_parsed_repo - if [[ -z "$remote_url" || -z "$result_var" ]]; then + if [[ -z "$__gh_remote_url" || -z "$__gh_result_name" ]]; then log_error "Usage: gh_repo_from_remote_url " return 1 fi - assert_variable_name "$result_var" + assert_variable_name "$__gh_result_name" - case "$remote_url" in + case "$__gh_remote_url" in git@github.com:*.git) - parsed_repo="${remote_url#git@github.com:}" - parsed_repo="${parsed_repo%.git}" + __gh_parsed_repo="${__gh_remote_url#git@github.com:}" + __gh_parsed_repo="${__gh_parsed_repo%.git}" ;; git@github.com:*) - parsed_repo="${remote_url#git@github.com:}" + __gh_parsed_repo="${__gh_remote_url#git@github.com:}" ;; https://github.com/*.git) - parsed_repo="${remote_url#https://github.com/}" - parsed_repo="${parsed_repo%.git}" + __gh_parsed_repo="${__gh_remote_url#https://github.com/}" + __gh_parsed_repo="${__gh_parsed_repo%.git}" ;; https://github.com/*) - parsed_repo="${remote_url#https://github.com/}" + __gh_parsed_repo="${__gh_remote_url#https://github.com/}" ;; *) return 1 ;; esac - [[ "$parsed_repo" == */* && "$parsed_repo" != */*/* ]] || return 1 - printf -v "$result_var" '%s' "$parsed_repo" + [[ "$__gh_parsed_repo" == */* && "$__gh_parsed_repo" != */*/* ]] || return 1 + printf -v "$__gh_result_name" '%s' "$__gh_parsed_repo" } gh_infer_repo_from_origin() { diff --git a/lib/bash/gh/tests/lib_gh.bats b/lib/bash/gh/tests/lib_gh.bats index bb4d77e..07f0fe3 100644 --- a/lib/bash/gh/tests/lib_gh.bats +++ b/lib/bash/gh/tests/lib_gh.bats @@ -200,6 +200,17 @@ EOF [ "$repo" = "owner/repo" ] } +@test "gh_repo_from_remote_url supports shadowing-prone output variable names" { + local result_var="" + local parsed_repo="" + + gh_repo_from_remote_url "https://github.com/owner/repo.git" result_var + gh_repo_from_remote_url "git@github.com:other/project.git" parsed_repo + + [ "$result_var" = "owner/repo" ] + [ "$parsed_repo" = "other/project" ] +} + @test "gh_repo_from_remote_url rejects non-GitHub and malformed remotes" { local repo="sentinel" diff --git a/lib/bash/git/lib_git.sh b/lib/bash/git/lib_git.sh index d28b445..5c5eb46 100644 --- a/lib/bash/git/lib_git.sh +++ b/lib/bash/git/lib_git.sh @@ -244,27 +244,27 @@ git_update_repo() { # - The function itself returns an exit code of 0 on success, 1 on invalid usage. # git_get_current_branch() { - local target_dir="$1" - local result_var_name="${2:-}" + local __git_branch_target_dir="$1" + local __git_branch_result_name="${2:-}" # --- Argument Validation --- - if [[ -z "$target_dir" || -z "$result_var_name" ]]; then + if [[ -z "$__git_branch_target_dir" || -z "$__git_branch_result_name" ]]; then log_error "Usage: git_get_current_branch " return 1 fi - if ! __is_valid_variable_name__ "$result_var_name"; then + if ! __is_valid_variable_name__ "$__git_branch_result_name"; then log_error "git_get_current_branch: result variable name must be a valid Bash variable name." return 1 fi - printf -v "$result_var_name" '%s' "" + printf -v "$__git_branch_result_name" '%s' "" - if [[ ! -d "$target_dir" ]]; then + if [[ ! -d "$__git_branch_target_dir" ]]; then return 0 fi # Check if we are inside a Git repository. - if ! git -C "$target_dir" rev-parse --is-inside-work-tree > /dev/null 2>&1; then + if ! git -C "$__git_branch_target_dir" rev-parse --is-inside-work-tree > /dev/null 2>&1; then # Not a Git repo, result is already an empty string. return 0 fi @@ -272,13 +272,13 @@ git_get_current_branch() { # Use 'git symbolic-ref' to get the branch name. # It's the most reliable way to distinguish a branch from a detached HEAD. # -q (--quiet) suppresses errors and returns a non-zero exit code on failure. - local branch_name - if branch_name=$(git -C "$target_dir" symbolic-ref --short -q HEAD); then + local __git_branch_name + if __git_branch_name=$(git -C "$__git_branch_target_dir" symbolic-ref --short -q HEAD); then # Success: We are on a named branch. - printf -v "$result_var_name" '%s' "$branch_name" + printf -v "$__git_branch_result_name" '%s' "$__git_branch_name" else # Failure: We are in a detached HEAD state. - printf -v "$result_var_name" '%s' "detached head" + printf -v "$__git_branch_result_name" '%s' "detached head" fi return 0 diff --git a/lib/bash/git/tests/lib_git.bats b/lib/bash/git/tests/lib_git.bats index 87b0c19..e3c311d 100644 --- a/lib/bash/git/tests/lib_git.bats +++ b/lib/bash/git/tests/lib_git.bats @@ -41,6 +41,20 @@ setup() { [ "$branch" = "main" ] } +@test "git_get_current_branch supports shadowing-prone output variable names" { + local repo="$TEST_TMPDIR/repo" + local result_var_name="" + local branch_name="" + + init_git_repo "$repo" + + git_get_current_branch "$repo" result_var_name + git_get_current_branch "$repo" branch_name + + [ "$result_var_name" = "main" ] + [ "$branch_name" = "main" ] +} + @test "git_get_current_branch reports detached head" { local repo="$TEST_TMPDIR/repo" local branch="" diff --git a/lib/bash/std/lib_std.sh b/lib/bash/std/lib_std.sh index a121679..8c2767e 100644 --- a/lib/bash/std/lib_std.sh +++ b/lib/bash/std/lib_std.sh @@ -1351,14 +1351,14 @@ std_register_cleanup_path() { ######################################################## TEMP FILES #################################################### __std_make_temp_path__() { - local helper_name="$1" path_kind="$2" + local __std_temp_helper_name="$1" __std_temp_path_kind="$2" shift 2 - local keep=0 result_name prefix temp_root template temp_path + local __std_temp_keep=0 __std_temp_result_name __std_temp_prefix __std_temp_root __std_temp_template __std_temp_path while (($#)); do case "${1-}" in --keep) - keep=1 + __std_temp_keep=1 shift ;; --) @@ -1372,50 +1372,50 @@ __std_make_temp_path__() { done if (($# < 1 || $# > 2)); then - log_error "$helper_name: usage: $helper_name [--keep] [prefix]" + log_error "$__std_temp_helper_name: usage: $__std_temp_helper_name [--keep] [prefix]" return 1 fi - result_name="$1" - prefix="${2:-base-bash-libs}" + __std_temp_result_name="$1" + __std_temp_prefix="${2:-base-bash-libs}" - if ! __is_valid_variable_name__ "$result_name"; then - log_error "$helper_name: result variable name must be a valid Bash variable name." + if ! __is_valid_variable_name__ "$__std_temp_result_name"; then + log_error "$__std_temp_helper_name: result variable name must be a valid Bash variable name." return 1 fi - if [[ -z "$prefix" || "$prefix" == */* ]]; then - log_error "$helper_name: prefix must be a non-empty filename prefix without '/'." + if [[ -z "$__std_temp_prefix" || "$__std_temp_prefix" == */* ]]; then + log_error "$__std_temp_helper_name: prefix must be a non-empty filename prefix without '/'." return 1 fi - temp_root="${TMPDIR:-/tmp}" - temp_root="${temp_root%/}" - if [[ -z "$temp_root" || ! -d "$temp_root" ]]; then - log_error "$helper_name: TMPDIR is not a directory: ${TMPDIR:-/tmp}" + __std_temp_root="${TMPDIR:-/tmp}" + __std_temp_root="${__std_temp_root%/}" + if [[ -z "$__std_temp_root" || ! -d "$__std_temp_root" ]]; then + log_error "$__std_temp_helper_name: TMPDIR is not a directory: ${TMPDIR:-/tmp}" return 1 fi - template="$temp_root/$prefix.XXXXXXXXXX" - if [[ "$path_kind" == "dir" ]]; then - temp_path="$(mktemp -d "$template" 2>/dev/null)" || { - log_error "$helper_name: failed to create temporary directory." + __std_temp_template="$__std_temp_root/$__std_temp_prefix.XXXXXXXXXX" + if [[ "$__std_temp_path_kind" == "dir" ]]; then + __std_temp_path="$(mktemp -d "$__std_temp_template" 2>/dev/null)" || { + log_error "$__std_temp_helper_name: failed to create temporary directory." return 1 } else - temp_path="$(mktemp "$template" 2>/dev/null)" || { - log_error "$helper_name: failed to create temporary file." + __std_temp_path="$(mktemp "$__std_temp_template" 2>/dev/null)" || { + log_error "$__std_temp_helper_name: failed to create temporary file." return 1 } fi - if ((! keep)); then - if ! std_register_cleanup_path "$temp_path"; then - rm -rf -- "$temp_path" + if ((! __std_temp_keep)); then + if ! std_register_cleanup_path "$__std_temp_path"; then + rm -rf -- "$__std_temp_path" return 1 fi fi - printf -v "$result_name" '%s' "$temp_path" + printf -v "$__std_temp_result_name" '%s' "$__std_temp_path" return 0 } @@ -1522,22 +1522,22 @@ assert_indexed_array() { # fi # std_command_path() { - local result_name="${1-}" command_name="${2-}" resolved_path="" + local __std_command_result_name="${1-}" __std_command_name="${2-}" __std_command_resolved_path="" if (($# != 2)); then log_error "std_command_path: usage: std_command_path " return 1 fi - if ! __is_valid_variable_name__ "$result_name"; then + if ! __is_valid_variable_name__ "$__std_command_result_name"; then log_error "std_command_path: result variable name must be a valid Bash variable name." return 1 fi - if [[ -n "$command_name" ]]; then - resolved_path="$(type -P "$command_name" 2>/dev/null || true)" + if [[ -n "$__std_command_name" ]]; then + __std_command_resolved_path="$(type -P "$__std_command_name" 2>/dev/null || true)" fi - printf -v "$result_name" '%s' "$resolved_path" - [[ -n "$resolved_path" ]] + printf -v "$__std_command_result_name" '%s' "$__std_command_resolved_path" + [[ -n "$__std_command_resolved_path" ]] } # @@ -1882,16 +1882,16 @@ safe_unalias() { # get_my_source_dir var_name # get_my_source_dir() { - local result_name="${1-}" - [[ -n "$result_name" ]] || fatal_error "get_my_source_dir: No result variable name provided." - if ! __is_valid_variable_name__ "$result_name"; then + local __std_source_result_name="${1-}" + [[ -n "$__std_source_result_name" ]] || fatal_error "get_my_source_dir: No result variable name provided." + if ! __is_valid_variable_name__ "$__std_source_result_name"; then fatal_error "get_my_source_dir: result variable name must be a valid Bash variable name." fi - local source_dir + local __std_source_dir # Reference: https://stackoverflow.com/a/246128/6862601 - source_dir="$(cd "$(dirname "${BASH_SOURCE[1]}")" >/dev/null 2>&1 && pwd -P)" || + __std_source_dir="$(cd "$(dirname "${BASH_SOURCE[1]}")" >/dev/null 2>&1 && pwd -P)" || fatal_error "get_my_source_dir: Unable to resolve source directory." - printf -v "$result_name" '%s' "$source_dir" + printf -v "$__std_source_result_name" '%s' "$__std_source_dir" } # diff --git a/lib/bash/std/tests/lib_std.bats b/lib/bash/std/tests/lib_std.bats index a0a427e..16d145a 100644 --- a/lib/bash/std/tests/lib_std.bats +++ b/lib/bash/std/tests/lib_std.bats @@ -1547,6 +1547,21 @@ EOF [ -f "$created_path" ] } +@test "std_make_temp helpers support shadowing-prone output variable names" { + local temp_root="$TEST_TMPDIR/temp-shadow-root" + local temp_path="" + local result_name="" + + mkdir -p "$temp_root" + TMPDIR="$temp_root" std_make_temp_file --keep temp_path shadow-file + TMPDIR="$temp_root" std_make_temp_dir --keep result_name shadow-dir + + [[ "$temp_path" == "$temp_root"/shadow-file.* ]] + [ -f "$temp_path" ] + [[ "$result_name" == "$temp_root"/shadow-dir.* ]] + [ -d "$result_name" ] +} + @test "std_make_temp helpers reject invalid result variable names" { local stderr_file="$TEST_TMPDIR/temp-invalid.err" local rc @@ -1585,6 +1600,20 @@ EOF [ "$command_path" = "" ] } +@test "std_command_path supports shadowing-prone output variable names" { + local result_name="" + local command_name="" + local resolved_path="" + + std_command_path result_name bash + std_command_path command_name bash + std_command_path resolved_path bash + + [ -x "$result_name" ] + [ -x "$command_name" ] + [ -x "$resolved_path" ] +} + @test "std_command_path rejects invalid result variable names" { local stderr_file="$TEST_TMPDIR/command-path.err" local rc @@ -2031,6 +2060,30 @@ EOF [[ "$output" == *"dir=$expected_dir"* ]] } +@test "get_my_source_dir supports shadowing-prone output variable names" { + local script="$TEST_TMPDIR/get-source-dir-shadow.sh" + local expected_dir + + create_script "$script" <