diff --git a/.github/workflows/verify-split-files.yml b/.github/workflows/verify-split-files.yml index 955961e..c5ef365 100644 --- a/.github/workflows/verify-split-files.yml +++ b/.github/workflows/verify-split-files.yml @@ -64,6 +64,7 @@ on: - "tests/gpg-key-reason.sh" - "tests/forge-branch-clean-install.sh" - "tests/alpine-clean-install.sh" + - "tests/app-vars-files.sh" - ".github/workflows/verify-split-files.yml" push: branches: [main] @@ -119,6 +120,7 @@ on: - "tests/gpg-key-reason.sh" - "tests/forge-branch-clean-install.sh" - "tests/alpine-clean-install.sh" + - "tests/app-vars-files.sh" - ".github/workflows/verify-split-files.yml" workflow_dispatch: @@ -245,6 +247,8 @@ jobs: run: bash tests/forge-branch-clean-install.sh - name: Clean install on Alpine tests run: bash tests/alpine-clean-install.sh + - name: App .vars file tests + run: bash tests/app-vars-files.sh - name: Prefetch lists cover every engine file run: | @@ -317,6 +321,23 @@ jobs: check ui/build-ui.func ui/API.txt || fail=1 check api/api.func api/API.txt || fail=1 exit $fail + - name: Every var_* the engine reads is classified + # An app's own .vars file may set any var_* outside VAR_RESERVED, so each + # one the engine reads has to be placed: VAR_WHITELIST (validated), + # VAR_RESERVED (never from a file), VAR_PER_RUN (never saved from a run) + # or FILE_OK (harmless unvalidated). + run: | + set -euo pipefail + FILE_OK="var_docker_lxcfs var_extra_disk var_extra_disk_path var_profile var_ssh_import_glob" + LOCALS="var_key var_val" + listed=" $(sed -n '/^declare -ag VAR_\(WHITELIST\|RESERVED\|PER_RUN\)=(/,/^)/p' ui/defaults.func | grep -oE '\bvar_[a-z0-9_]+' | tr '\n' ' ') $FILE_OK $LOCALS " + fail=0 + for v in $(grep -rhoE '\$\{?!?#?var_[a-z0-9_]+' $ENGINE_DIRS --include='*.func' | grep -oE 'var_[a-z0-9_]+' | sort -u); do + [[ "$listed" == *" $v "* ]] && continue + echo "::error file=ui/defaults.func::$v is read by the engine but in neither VAR_WHITELIST, VAR_RESERVED nor FILE_OK" + fail=1 + done + exit $fail - name: Update mode prompt tests run: bash tests/update-mode-prompt.sh diff --git a/docs/testing-builds.md b/docs/testing-builds.md index 2968ce4..475e6fa 100644 --- a/docs/testing-builds.md +++ b/docs/testing-builds.md @@ -72,7 +72,7 @@ over. ## Two things left out on purpose -**Not in `VAR_WHITELIST`.** `var_testurl` is not settable from a `.vars` file. +**In `VAR_RESERVED`.** `var_testurl` is not settable from a `.vars` file. It describes the script, not the user's preferences, and making it configurable would mainly serve as a way to switch the request off. diff --git a/docs/ui.md b/docs/ui.md index c3228ee..9d1fce4 100644 --- a/docs/ui.md +++ b/docs/ui.md @@ -32,6 +32,12 @@ Where settings come from and where they are kept: storage selection, the global and per-app `.vars` files, app defaults, and the diff shown when a saved default no longer matches what the script wants. +`default.vars` takes the keys in `VAR_WHITELIST`, each one validated. An app's +own `defaults/.vars` also takes the app's own settings (any other `var_*`) +as they are, since only the script knows what they mean. Keys in +`VAR_RESERVED` are never read from a file. Saving app defaults keeps the app's +own settings, but never takes the keys in `VAR_PER_RUN` from the current run. + ## [`advanced.func`](../ui/advanced.func) `advanced_settings` — one function, the full interactive configuration path. diff --git a/tests/app-vars-files.sh b/tests/app-vars-files.sh new file mode 100644 index 0000000..fde7e47 --- /dev/null +++ b/tests/app-vars-files.sh @@ -0,0 +1,71 @@ +#!/usr/bin/env bash +set -euo pipefail + +CORE_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")/.." && pwd)" +DEFAULTS="$CORE_DIR/ui/defaults.func" +eval "$(awk '/^declare -ag VAR_(WHITELIST|RESERVED|PER_RUN)=\(/,/^\)/' "$DEFAULTS")" +for fn in _is_whitelisted_key _is_reserved_var_key load_vars_file _load_vars_file_to_map; do + eval "$(awk "/^${fn}\\(\\) \\{/,/^}/" "$DEFAULTS")" +done + +WORK="$(mktemp -d)" +trap 'rm -rf "$WORK"' EXIT +WARNINGS="$WORK/warnings" +msg_info() { :; } +msg_ok() { :; } +msg_warn() { echo "$*" >>"$WARNINGS"; } +passed=0 failed=0 + +check() { + if [[ "$3" == "$2" ]]; then + echo "PASS $1" + passed=$((passed + 1)) + else + echo "FAIL $1: expected <$2>, got <$3>" >&2 + failed=$((failed + 1)) + fi +} + +cat >"$WORK/test.vars" <<'EOF' +var_cpu=4 +var_foo_port=8080 +var_foo_name="My App" +var_ctid=150 +var_external_installer=yes +var_Upper=1 +var_appversion=1.2.3 +EOF + +# $1: app_keys; prints the variables a load leaves set +loaded() { + ( + : >"$WARNINGS" + load_vars_file "$WORK/test.vars" no "" "$1" + printf '%s|' "${var_cpu:-}" "${var_foo_port:-}" "${var_foo_name:-}" "${var_ctid:-}" "${var_external_installer:-}" "${var_Upper:-}" + ) +} + +check "default.vars takes whitelisted keys only" "4||||||" "$(loaded no)" +check "an app file also takes the app's own keys" "4|8080|My App||||" "$(loaded yes)" +loaded yes >/dev/null +check "each reserved key is reported" 2 "$(grep -c 'is ignored' "$WARNINGS")" +check "a per-run key written into an app file by hand loads" 1.2.3 "$(load_vars_file "$WORK/test.vars" no "" yes && echo "${var_appversion:-}")" +check "default.vars does not take a per-run key" "" "$(load_vars_file "$WORK/test.vars" no "" no && echo "${var_appversion:-}")" + +_load_vars_file_to_map "$WORK/test.vars" +check "the map keeps the app's own keys" 8080 "${_VARS_IN[var_foo_port]:-}" +check "the map drops reserved keys" "" "${_VARS_IN[var_ctid]:-}${_VARS_IN[var_external_installer]:-}" + +for key in "${VAR_WHITELIST[@]}" "${VAR_PER_RUN[@]}"; do + if _is_reserved_var_key "$key"; then + check "$key is in one list only" no yes + fi +done +for key in "${VAR_PER_RUN[@]}"; do + if _is_whitelisted_key "$key"; then + check "$key is in one list only" no yes + fi +done + +printf '%s passed, %s failed\n' "$passed" "$failed" +((failed == 0)) diff --git a/ui/API.txt b/ui/API.txt index 405cff4..f2a3d86 100644 --- a/ui/API.txt +++ b/ui/API.txt @@ -50,6 +50,7 @@ _eh_write_errinfo _get_current_ip _in_zone_tab _is_container_context +_is_reserved_var_key _is_whitelisted_key _list_os_versions _list_templates diff --git a/ui/defaults.func b/ui/defaults.func index 64548f0..04bdc7b 100644 --- a/ui/defaults.func +++ b/ui/defaults.func @@ -404,12 +404,50 @@ base_settings() { resolve_os_version } +# ------------------------------------------------------------------------------ +# var_* keys in .vars files +# +# VAR_WHITELIST: engine settings; load_vars_file validates each one. +# VAR_RESERVED: never taken from a file, not even an app's own. Unique per +# container, declared by the script, read before any file is loaded, or a +# safety override that has to be asked for on the run that needs it. +# VAR_PER_RUN: an app file may hold them, but saving app defaults never takes +# them from the current run; they describe that run, not the app. +# Any other var_* is an app's own setting and loads only from its app file. +# The frontend's .vars export mirrors VAR_WHITELIST and VAR_RESERVED. +# ------------------------------------------------------------------------------ +declare -ag VAR_WHITELIST=( + var_apt_cacher var_apt_cacher_ip var_brg var_cmode var_cpu var_disk var_fuse var_github_token var_gpu var_http_no_proxy var_http_proxy var_inherit_host_ca var_keyctl + var_gateway var_hostname var_ipv6_method var_mac var_mknod var_mount_fs var_mtu + var_net var_nesting var_ns var_os var_post_install var_protection var_pw var_ram var_sdn_vnet var_searchdomain var_tags var_timezone var_tun var_unprivileged + var_verbose var_version var_vlan var_ssh var_ssh_authorized_key var_container_storage var_template_storage +) +declare -ag VAR_RESERVED=( + var_arm64 var_bridge var_ctid var_external_installer var_force_breaking_update var_ignore_breaking_changes + var_ignore_disable var_ignore_os_mismatch var_install var_ipv6_static var_testurl +) +declare -ag VAR_PER_RUN=( + var_appversion var_full_verbose var_unattended +) + +_is_whitelisted_key() { + local k="$1" w + for w in "${VAR_WHITELIST[@]}"; do [[ "$k" == "$w" ]] && return 0; done + return 1 +} + +_is_reserved_var_key() { + local k="$1" w + for w in "${VAR_RESERVED[@]}"; do [[ "$k" == "$w" ]] && return 0; done + return 1 +} + # ------------------------------------------------------------------------------ # load_vars_file() # # - Safe parser for KEY=VALUE lines from vars files # - Used by default_var_settings and app defaults loading -# - Only loads whitelisted var_* keys +# - Loads whitelisted var_* keys; with app_keys=yes also the app's own ones # - Optional force parameter to override existing values (for app defaults) # - Optional protected list preserves genuinely user-exported var_* values # ------------------------------------------------------------------------------ @@ -417,24 +455,10 @@ load_vars_file() { local file="$1" local force="${2:-no}" # If "yes", override existing variables local protected="${3:-}" # space-separated var_* keys the user genuinely exported before this file loaded; never overwritten + local app_keys="${4:-no}" # If "yes", also load var_* outside the whitelist (the app's own file) [ -f "$file" ] || return 0 msg_info "Loading defaults from ${file}" - # Allowed var_* keys - local VAR_WHITELIST=( - var_apt_cacher var_apt_cacher_ip var_brg var_cmode var_cpu var_disk var_fuse var_github_token var_gpu var_http_no_proxy var_http_proxy var_inherit_host_ca var_keyctl - var_gateway var_hostname var_ipv6_method var_mac var_mknod var_mount_fs var_mtu - var_net var_nesting var_ns var_os var_post_install var_protection var_pw var_ram var_sdn_vnet var_searchdomain var_tags var_timezone var_tun var_unprivileged - var_verbose var_version var_vlan var_ssh var_ssh_authorized_key var_container_storage var_template_storage - ) - - # Whitelist check helper - _is_whitelisted() { - local k="$1" w - for w in "${VAR_WHITELIST[@]}"; do [ "$k" = "$w" ] && return 0; done - return 1 - } - # Protected check helper (genuinely user-exported vars, see $protected above) _is_protected() { local k="$1" p @@ -452,7 +476,13 @@ load_vars_file() { local var_val="${BASH_REMATCH[2]}" [[ "$var_key" != var_* ]] && continue - _is_whitelisted "$var_key" || continue + if _is_reserved_var_key "$var_key"; then + msg_warn "${var_key} in ${file} is ignored: set it on the command line of the run that needs it" + continue + fi + if ! _is_whitelisted_key "$var_key"; then + [[ "$app_keys" == "yes" && "$var_key" =~ ^var_[a-z0-9_]+$ ]] || continue + fi # Strip inline comments (anything after unquoted #) # Only strip if not inside quotes @@ -678,15 +708,6 @@ load_vars_file() { # - Calls base_settings "$VERBOSE" and echo_default # ------------------------------------------------------------------------------ default_var_settings() { - # Allowed var_* keys (alphabetically sorted) - # Note: Removed var_ctid (can only exist once), var_ipv6_static (static IPs are unique) - local VAR_WHITELIST=( - var_apt_cacher var_apt_cacher_ip var_brg var_cmode var_cpu var_disk var_fuse var_github_token var_gpu var_http_no_proxy var_http_proxy var_inherit_host_ca var_keyctl - var_gateway var_hostname var_ipv6_method var_mac var_mknod var_mount_fs var_mtu - var_net var_nesting var_ns var_os var_post_install var_protection var_pw var_ram var_sdn_vnet var_searchdomain var_tags var_timezone var_tun var_unprivileged - var_verbose var_version var_vlan var_ssh var_ssh_authorized_key var_container_storage var_template_storage - ) - # Snapshot: environment variables (highest precedence) declare -A _HARD_ENV=() local _k @@ -806,15 +827,6 @@ EOF # Silent creation - no output message } - # Whitelist check - local _is_whitelisted_key - _is_whitelisted_key() { - local k="$1" - local w - for w in "${VAR_WHITELIST[@]}"; do [ "$k" = "$w" ] && return 0; done - return 1 - } - # 1) Ensure file exists _ensure_default_vars @@ -862,33 +874,6 @@ get_app_defaults_path() { echo "${base}/defaults/${n}.vars" } -# ------------------------------------------------------------------------------ -# maybe_offer_save_app_defaults -# -# - Called after advanced_settings returned with fully chosen values. -# - If no .vars exists, offers to persist current advanced settings -# into /usr/local/community-scripts/defaults/.vars -# - Only writes whitelisted var_* keys. -# - Extracts raw values from flags like ",gw=..." ",mtu=..." etc. -# ------------------------------------------------------------------------------ -if ! declare -p VAR_WHITELIST >/dev/null 2>&1; then - # Note: Removed var_ctid (can only exist once), var_ipv6_static (static IPs are unique) - declare -ag VAR_WHITELIST=( - var_apt_cacher var_apt_cacher_ip var_brg var_cmode var_cpu var_disk var_fuse var_github_token var_gpu var_http_no_proxy var_http_proxy var_inherit_host_ca var_keyctl - var_gateway var_hostname var_ipv6_method var_mac var_mknod var_mount_fs var_mtu - var_net var_nesting var_ns var_os var_post_install var_protection var_pw var_ram var_sdn_vnet var_searchdomain var_tags var_timezone var_tun var_unprivileged - var_verbose var_version var_vlan var_ssh var_ssh_authorized_key var_container_storage var_template_storage - ) -fi - -# Global whitelist check function (used by _load_vars_file_to_map and others) -_is_whitelisted_key() { - local k="$1" - local w - for w in "${VAR_WHITELIST[@]}"; do [ "$k" = "$w" ] && return 0; done - return 1 -} - _sanitize_value() { # Disallow Command-Substitution / Shell-Meta case "$1" in @@ -927,9 +912,7 @@ _load_vars_file_to_map() { val=$(printf "%s" "$line" | cut -d= -f2-) case "$key" in var_*) - if _is_whitelisted_key "$key"; then - _VARS_IN["$key"]="$val" - fi + _is_reserved_var_key "$key" || _VARS_IN["$key"]="$val" ;; esac done <"$file" @@ -1105,6 +1088,30 @@ _build_current_app_vars_tmp() { [ -n "$_tpl_storage" ] && echo "var_template_storage=$(_sanitize_value "$_tpl_storage")" [ -n "$_ct_storage" ] && echo "var_container_storage=$(_sanitize_value "$_ct_storage")" [ -n "${var_post_install:-}" ] && echo "var_post_install=$(_sanitize_value "${var_post_install}")" + + # The app's own settings are not in the dialog: keep what the file holds, + # and take what this run was started with. + local -A _own=() + local _k _f + _f="$(get_app_defaults_path)" + if [[ -f "$_f" ]]; then + _load_vars_file_to_map "$_f" + for _k in "${!_VARS_IN[@]}"; do + _is_whitelisted_key "$_k" || _own["$_k"]="${_VARS_IN[$_k]}" + done + fi + for _k in $(compgen -e var_); do + [[ "$_k" =~ ^var_[a-z0-9_]+$ && "${!_k}" != *$'\n'* ]] || continue + if _is_whitelisted_key "$_k" || _is_reserved_var_key "$_k" || [[ " ${VAR_PER_RUN[*]} " == *" $_k "* ]]; then continue; fi + _own["$_k"]="\"${!_k}\"" + done + if ((${#_own[@]} > 0)); then + echo + echo "# ${APP}'s own settings" + while IFS= read -r _k; do + echo "${_k}=${_own[$_k]}" + done < <(printf '%s\n' "${!_own[@]}" | sort) + fi } >"$tmpf" echo "$tmpf" @@ -1131,7 +1138,7 @@ maybe_offer_save_app_defaults() { if whiptail --backtitle "Proxmox VE Helper Scripts" \ --yesno "Save these advanced settings as defaults for ${APP}?\n\nThis will create:\n${app_vars_path}" 12 72; then mkdir -p "$(dirname "$app_vars_path")" - install -m 0644 "$new_tmp" "$app_vars_path" + install -m 0600 "$new_tmp" "$app_vars_path" msg_ok "Saved app defaults: ${app_vars_path}" fi rm -f "$new_tmp" "$diff_tmp" @@ -1165,7 +1172,7 @@ maybe_offer_save_app_defaults() { case "$sel" in "Update Defaults") - install -m 0644 "$new_tmp" "$app_vars_path" + install -m 0600 "$new_tmp" "$app_vars_path" msg_ok "Updated app defaults: ${app_vars_path}" break ;; diff --git a/ui/menu.func b/ui/menu.func index 60cc9ba..8009a3a 100644 --- a/ui/menu.func +++ b/ui/menu.func @@ -334,7 +334,7 @@ install_script() { header_info echo -e "${DEFAULT}${BOLD}${BL}Using App Defaults for ${APP} on node $PVEHOST_NAME${CL}" METHOD="appdefaults" - load_vars_file "$(get_app_defaults_path)" "yes" # Force override script defaults + load_vars_file "$(get_app_defaults_path)" "yes" "" "yes" # Force override script defaults, the app's own settings included base_settings echo_default defaults_target="$(get_app_defaults_path)"