diff --git a/SECURITY.md b/SECURITY.md index 82bbe1a..0badc0f 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -6,8 +6,9 @@ We provide security updates for the following versions: | Version | Supported | | ------- | ------------------ | +| 2.4.x | :white_check_mark: | | 2.3.x | :white_check_mark: | -| 2.2.x | :white_check_mark: | +| 2.2.x | :x: | | 2.1.x | :x: | | 2.0.x | :x: | | 1.3.x | :x: | diff --git a/demo-scripts/demo_unsafe_newlines.sh b/demo-scripts/demo_unsafe_newlines.sh index dc066b7..24f1332 100755 --- a/demo-scripts/demo_unsafe_newlines.sh +++ b/demo-scripts/demo_unsafe_newlines.sh @@ -46,7 +46,7 @@ demo_header "Log Injection Prevention Demo" # Create temporary files for this demo DEMO_TEMP_DIR=$(mktemp -d -t bash-logger-demo.XXXXXX) -trap 'rm -rf $DEMO_TEMP_DIR' EXIT +trap 'rm -rf -- "$DEMO_TEMP_DIR"' EXIT echo "This demo illustrates how bash-logger protects against log injection attacks" echo "by sanitizing newline characters in log messages." diff --git a/docs/security-reviews/2026-04-18-claude-security-audit-v2.4.0.md b/docs/security-reviews/2026-04-18-claude-security-audit-v2.4.0.md new file mode 100644 index 0000000..72400dd --- /dev/null +++ b/docs/security-reviews/2026-04-18-claude-security-audit-v2.4.0.md @@ -0,0 +1,525 @@ +# bash-logger v2.4.0 Security Audit + +**Auditor:** Senior Security Researcher (Claude AI) +**Date:** 2026-04-18 +**Component:** `logging.sh` v2.4.0 +**Previous Audit:** 2026-02-13 (v2.1.2) — all findings resolved, posture rated EXCELLENT +**Severity Scale:** CRITICAL | HIGH | MEDIUM | LOW | INFO + +--- + +## Executive Summary + +This audit examines bash-logger v2.4.0, covering all changes introduced since the v2.1.2 +follow-up audit (February 2026). The review focused on new features introduced in v2.2.x +through v2.4.0: selective journal logging (`log_to_journal`), syslog facility support, and +the init-message suppression option. + +**Overall Security Posture:** EXCELLENT — unchanged from v2.1.2 + +**Findings Summary:** + +| ID | Severity | Type | Title | +| ------- | -------- | ----------- | -------------------------------------------------------------------------- | +| BUG-01 | LOW | Bug | `log_init()` messages unconditionally route to stderr | +| BUG-02 | LOW | Bug | `set_journal_tag()` bypasses all input validation | +| BUG-03 | LOW | Bug | `_strip_ansi_codes()` leaves DCS/PM/APC sequence bodies intact | +| BUG-04 | LOW | Bug | Pre-set `LOGGER_*_ERROR_REPORTED` env vars suppress write-failure warnings | +| INFO-01 | INFO | Docs | `SECURITY.md` supported-versions table not updated for v2.4.0 | +| FEAT-01 | — | Enhancement | Validate boolean input in `set_unsafe_allow_*()` functions | +| FEAT-02 | — | Enhancement | `set_color_mode()` should reject unrecognised mode values | + +* 0 Critical vulnerabilities +* 0 High vulnerabilities +* 0 Medium vulnerabilities +* 4 Low vulnerabilities +* 1 Informational / documentation item +* 2 Enhancement recommendations + +**No findings block production use.** + +--- + +## Scope and Methodology + +### Changes reviewed since v2.1.2 + +* `log_to_journal()` — selective journal dispatch (v2.2.1) +* `set_syslog_facility()` / `--facility` option — syslog facility support (v2.3.0) +* `LOG_INIT_MESSAGE` / `--no-init-message` option — init-message suppression (v2.4.0) +* All runtime configuration setter functions +* `_strip_ansi_codes()` — ANSI sanitisation completeness +* Environment variable protection coverage + +### Methodology + +* Full static analysis of `logging.sh` source +* Cross-referencing runtime setters against config-parser validation paths +* Review of all public API functions for input validation consistency +* Differential analysis against previous audit findings +* Review of associated test coverage for new features + +--- + +## Findings + +### [BUG-01] `log_init()` Messages Unconditionally Route to stderr + +**Severity:** LOW +**Component:** `log_init()`, `_should_use_stderr()` +**CWE:** CWE-670 (Always-Incorrect Control Flow Implementation) + +**Description:** + +`log_init()` passes `-1` as the level value to `_log_message()` so that INIT messages +always bypass the log-level filter (the intent). However, `-1` also causes `_should_use_stderr()` +to unconditionally return `true`, overriding the user's `--stderr-level` configuration: + +```bash +# log_init() call: +log_init() { + _log_message "INIT" -1 "$1" # -1 bypasses level filter +} + +# _should_use_stderr() check: +_should_use_stderr() { + local level_value="$1" + [[ "$level_value" -le "$LOG_STDERR_LEVEL" ]] # -1 <= any positive value → always true +} +``` + +With the default `LOG_STDERR_LEVEL=3` (ERROR): `[[ -1 -le 3 ]]` is always true. Even +`--stderr-level EMERGENCY` (level 0) results in `[[ -1 -le 0 ]]` → true. + +**Impact:** + +* INIT messages always appear on stderr regardless of configuration +* Users who configure strict stderr levels (e.g., only EMERGENCY) cannot prevent INIT + messages from polluting stderr +* Scripts that parse stderr for errors will receive false positives from INIT messages +* Breaks the user's ability to separate informational init output from actual errors + +**Affected Versions:** All versions using the `-1` sentinel pattern (v1.x+) + +**Proof of Concept:** + +```bash +source logging.sh +init_logger --stderr-level EMERGENCY --no-color + +# This should not appear on stderr — but it does: +log_init "Application starting" 2>/tmp/stderr_test.txt >/dev/null +cat /tmp/stderr_test.txt # INIT message appears despite EMERGENCY threshold +``` + +**Recommended Fix:** + +Option A (preferred) — give INIT a real level constant and bypass the filter explicitly: + +```bash +# At constant initialisation: +readonly LOG_LEVEL_INIT=6 # Treat as INFO-equivalent for routing purposes + +# In log_init(): +log_init() { + # Skip level filter but preserve normal stderr routing + local saved_level="$CURRENT_LOG_LEVEL" + CURRENT_LOG_LEVEL=$LOG_LEVEL_DEBUG # temporarily allow all + _log_message "INIT" $LOG_LEVEL_INIT "$1" + CURRENT_LOG_LEVEL="$saved_level" +} +``` + +Option B (minimal) — guard the stderr check against negative sentinel values: + +```bash +_should_use_stderr() { + local level_value="$1" + [[ "$level_value" -ge 0 && "$level_value" -le "$LOG_STDERR_LEVEL" ]] +} +``` + +**Note:** `log_sensitive()` uses `skip_file=true, skip_journal=true` to control output +routing before `_should_use_stderr()` is consulted, so it is unaffected by this issue. + +--- + +### [BUG-02] `set_journal_tag()` Bypasses All Input Validation + +**Severity:** LOW +**Component:** `set_journal_tag()` +**CWE:** CWE-20 (Improper Input Validation) + +**Description:** + +`set_journal_tag()` assigns the caller-supplied value directly without any validation: + +```bash +set_journal_tag() { + local old_tag="$JOURNAL_TAG" + JOURNAL_TAG="$1" # ← no length check, no metacharacter check + ... +} +``` + +In contrast, the configuration-file parser path calls `_validate_journal_tag()`, which +enforces a 64-character maximum length and rejects shell metacharacters. The runtime setter +silently bypasses both checks. + +The journal tag is passed to the `logger` executable with proper quoting +(`"$LOGGER_PATH" ... -t "$tag"`), so shell command injection is not possible. However: + +* Tags longer than 64 characters exceed systemd-journal's limit and may be silently truncated + or rejected, producing an unreliable audit trail +* Tags containing control characters (including embedded NUL bytes) may corrupt journal indices + or confuse log-analysis tooling +* A value rejected by the config-file parser can be set unimpeded via the runtime API, + undermining users' reasonable expectation of consistent validation across both paths + +**Impact:** Unreliable journal tagging when tags are set programmatically; misleading audit +trail; inconsistent security boundary between configuration paths. + +**Proof of Concept:** + +```bash +source logging.sh +init_logger --journal --no-color + +# Config file would reject both of these; set_journal_tag() accepts both silently: +set_journal_tag "$(printf 'A%.0s' {1..200})" # 200 chars, no error +set_journal_tag $'valid\x00hidden' # embedded NUL, no error +echo "Tag length: ${#JOURNAL_TAG}" # 200 +``` + +**Recommended Fix:** + +Add a call to the existing `_validate_journal_tag()` at the top of `set_journal_tag()`: + +```bash +set_journal_tag() { + if ! _validate_journal_tag "$1" 0; then # 0 = not from config file + return 1 + fi + local old_tag="$JOURNAL_TAG" + JOURNAL_TAG="$1" + ... +} +``` + +This mirrors the pattern already established by `set_syslog_facility()` calling +`_validate_syslog_facility()`. + +--- + +### [BUG-03] `_strip_ansi_codes()` Leaves DCS, PM, and APC Sequence Bodies Intact + +**Severity:** LOW +**Component:** `_strip_ansi_codes()` +**CWE:** CWE-150 (Improper Neutralization of Escape, Meta, or Control Sequences) + +**Description:** + +`_strip_ansi_codes()` uses a multi-pass sed approach: + +* **Step 1:** Removes CSI sequences (`\e[...letter`) — complete +* **Step 2:** Removes OSC sequences (`\e]...BEL` and `\e]...\e\\`) — complete +* **Step 3:** Removes two-character escape sequences (`\eX` where X ≠ `[`) — _partial_ + +Step 3 correctly removes the two-byte introducer for Device Control String (`\eP`), Privacy +Message (`\e^`), and Application Program Command (`\e_`) sequences, but the payload data and +ST terminator (`...data...\e\\`) that follows each opener is left in the output. Only OSC +(`\e]`) receives full body-stripping treatment in step 2. + +**Impact:** + +* Payload bytes from DCS, PM, and APC sequences survive sanitisation and appear in console + output and log files +* DCS sequences are used by terminals for sixel graphics and Tmux passthrough; malicious + payloads could affect terminal state +* PM and APC sequences are used by terminal multiplexers; escaped payloads could exfiltrate + data via terminal query responses in interactive sessions +* Severity is LOW because exploitation requires attacker-controlled log input AND a terminal + emulator that acts on these sequences in the specific way + +**Proof of Concept:** + +```bash +source logging.sh +init_logger --no-color + +# DCS sequence: ESC P ESC \ +dcs=$'\ePsixel-data-here\e\\' +result=$(_strip_ansi_codes "$dcs") +# Expected: "" +# Actual: "sixel-data-here" + ESC + "\" remain in output + +# PM sequence: ESC ^ ESC \ +pm=$'\e^metadata\e\\' +result=$(_strip_ansi_codes "$pm") +# Actual: "metadata" + ESC + "\" remain +``` + +**Recommended Fix:** + +Insert additional passes after current step 2b, before the existing step 3, following the +same pattern used for OSC body stripping: + +```bash +# Remove DCS sequences (\eP...payload...\e\\) +local step2c +step2c=$(printf '%s' "$step2b" | sed "s|${esc}P[^${esc}]*${esc}\\\\||g") + +# Remove PM sequences (\e^...payload...\e\\) +local step2d +step2d=$(printf '%s' "$step2c" | sed "s|${esc}\\^[^${esc}]*${esc}\\\\||g") + +# Remove APC sequences (\e_...payload...\e\\) +local step2e +step2e=$(printf '%s' "$step2d" | sed "s|${esc}_[^${esc}]*${esc}\\\\||g") + +# Rename: existing step3 becomes step3 from step2e input +local step3 +step3=$(printf '%s' "$step2e" | sed 's/\x1b[^[]//g') +``` + +Additional regression tests should be added to `tests/test_ansi_injection.sh` covering +DCS, PM, and APC payloads. + +--- + +### [BUG-04] Pre-set `LOGGER_*_ERROR_REPORTED` Environment Variables Suppress Write-Failure Warnings + +**Severity:** LOW +**Component:** `_log_message()`, `_write_to_journal()` +**CWE:** CWE-693 (Protection Mechanism Failure) + +**Description:** + +`_log_message()` and `_write_to_journal()` each use a global string flag to print +write-failure error messages only once (preventing log spam): + +```bash +# In _log_message(): +if [[ -z "${LOGGER_FILE_ERROR_REPORTED:-}" ]]; then + echo "ERROR: Failed to write to log file" >&2 + LOGGER_FILE_ERROR_REPORTED="yes" +fi + +# In _write_to_journal(): +if [[ -z "${LOGGER_JOURNAL_ERROR_REPORTED:-}" ]]; then + echo "Warning: logger command unavailable ..." >&2 + LOGGER_JOURNAL_ERROR_REPORTED="yes" +fi +``` + +Neither variable is unset or reset at source time. They are not included in the +environment-variable protection block that already guards `LOG_LEVEL_*` and `COLOR_*` +constants. If either is pre-set to any non-empty value in the process environment before +`logging.sh` is sourced, all write-failure warnings for that subsystem are silently +suppressed for the entire process lifetime. + +An attacker with environment control (e.g., a compromised wrapper script, a crafted +`sudo` environment, or a CI pipeline with injected environment variables) can use: + +```bash +export LOGGER_FILE_ERROR_REPORTED=yes +export LOGGER_JOURNAL_ERROR_REPORTED=yes +``` + +to ensure any failure of log delivery is invisible to the calling script and its operators, +hiding evidence of tampering with log destinations. + +**Impact:** + +* Write failures (disk full, permissions changed, file deleted) are silently swallowed +* Operators lose the only in-band signal that logging has failed +* Particularly relevant in security-sensitive scripts where the absence of log-delivery + errors is treated as confirmation of successful audit logging + +**Proof of Concept:** + +```bash +export LOGGER_FILE_ERROR_REPORTED=injected + +source logging.sh +init_logger --log /tmp/probe.log --no-color +chmod 000 /tmp/probe.log # revoke write access after init + +log_error "This fails silently" +# Expected: "ERROR: Failed to write to log file" on stderr +# Actual: complete silence +``` + +**Recommended Fix:** + +Add unconditional unset calls in the source-time initialisation block, mirroring the +pattern used for `LOG_LEVEL_*` constants: + +```bash +# Near the top of logging.sh, in the initialisation section: +unset LOGGER_FILE_ERROR_REPORTED 2>/dev/null || true +unset LOGGER_JOURNAL_ERROR_REPORTED 2>/dev/null || true +``` + +These flags do not need `readonly` treatment because they are legitimately set to `"yes"` +during normal operation; only the initial value needs to be guaranteed empty on each source. + +--- + +### [INFO-01] `SECURITY.md` Supported Versions Table Not Updated for v2.4.0 + +**Severity:** INFO / Documentation +**Component:** `SECURITY.md` + +**Description:** + +Following the release of v2.4.0, `SECURITY.md` still lists `2.3.x` and `2.2.x` as the +most recent supported versions. Version `2.4.x` is absent from the table entirely, which +could lead users to incorrectly conclude that the current release is unsupported and +discourage them from updating. + +**Recommended Fix:** + +Add `2.4.x` to the supported versions table and review whether `2.2.x` remains supported +or should be marked `:x:`. + +--- + +### [FEAT-01] `set_unsafe_allow_newlines()` and `set_unsafe_allow_ansi_codes()` Should Validate Boolean Input + +**Severity:** Enhancement +**Component:** `set_unsafe_allow_newlines()`, `set_unsafe_allow_ansi_codes()` + +**Description:** + +Both unsafe-mode setters assign the caller's argument without normalisation: + +```bash +LOG_UNSAFE_ALLOW_NEWLINES="$1" +``` + +Values like `"True"`, `"yes"`, `"1"`, or `"enable"` are accepted silently. Because the +protection check is `!= "true"`, only the exact string `"true"` disables sanitisation — +so mistyped values leave protection _enabled_ — but the CONFIG log entry records the +incorrect value, producing a misleading audit trail. + +This contrasts with every other runtime setter: `set_syslog_facility()` validates against +a whitelist, `set_log_level()` normalises via `_get_log_level_value()`. + +**Recommended Enhancement:** + +Add the same boolean normalisation the config parser already uses (accepts `true/false`, +`yes/no`, `on/off`, `1/0`), returning non-zero for unrecognised input. + +--- + +### [FEAT-02] `set_color_mode()` Should Reject Unrecognised Mode Values + +**Severity:** Enhancement +**Component:** `set_color_mode()` + +**Description:** + +The `*)` wildcard case in `set_color_mode()` sets `USE_COLORS` to whatever string is +passed, with a comment implying it handles the canonical `always/never/auto` passthrough: + +```bash +*) + USE_COLORS="$mode" # Set directly if it's already "always", "never", or "auto" + ;; +``` + +An invalid value like `"foobar"` is silently accepted, stored, and written to the CONFIG +log entry, then falls through to `_detect_color_support()` in `_should_use_colors()`. +This produces an unreliable and misleading configuration audit trail. + +**Recommended Enhancement:** + +Replace the wildcard with explicit acceptance of `always`, `never`, and `auto`, and +return non-zero with a descriptive error for all other values. + +--- + +## Positive Security Observations + +The following security improvements introduced since v2.1.2 were reviewed and found to +be correctly implemented: + +### `log_to_journal()` — Secure Implementation + +The new selective-journal function correctly: + +* Validates the level name via an exhaustive `case` statement before any action +* Short-circuits silently when the message is below the current log level (no discovery side-effects) +* Uses the existing `_LOGGER_DISCOVERY_DONE` fast-path to avoid redundant path validation +* Performs an explicit `LOGGER_PATH` executability check before calling `_log_message()` +* Emits a single deduplication-guarded warning when `logger` is absent +* Passes `force_journal=true` to `_log_message()` correctly; `skip_journal` in `log_sensitive()` still takes precedence +* Prevents double-dispatch when `USE_JOURNAL=true` by passing the message through `_log_message` only once + +### Syslog Facility Support — Correct Validation + +`set_syslog_facility()` calls `_validate_syslog_facility()` before assignment and returns +non-zero on invalid input, following the correct defensive pattern. The facility is also +lowercased before storage and use. + +### `LOG_INIT_MESSAGE` — Correct Environment Isolation + +The new v2.4.0 variable uses an unconditional assignment (`LOG_INIT_MESSAGE="true"`) at +source time, which overrides any pre-existing environment value. This is the correct +approach and consistent with how `LOG_UNSAFE_ALLOW_NEWLINES` and `LOG_UNSAFE_ALLOW_ANSI_CODES` +are initialised. + +--- + +## Test Coverage Assessment + +| Finding | Existing Test Coverage | Recommended New Tests | +| ------- | -------------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------- | +| BUG-01 | None — no test for stderr routing of `log_init` | `test_initialization.sh`: assert `log_init` does not write to stderr when `--stderr-level EMERGENCY` | +| BUG-02 | `test_journal_logging.sh` covers `log_to_journal` but not `set_journal_tag` validation | `test_runtime_configuration.sh` (or new file): oversized tag, tag with control chars | +| BUG-03 | `test_ansi_injection.sh` covers CSI/OSC but not DCS/PM/APC bodies | Add DCS, PM, APC body-stripping tests to `test_ansi_injection.sh` | +| BUG-04 | `test_environment_security.sh` covers `LOG_LEVEL_*` and `COLOR_*` but not error flags | Add pre-set `LOGGER_FILE_ERROR_REPORTED` / `LOGGER_JOURNAL_ERROR_REPORTED` tests | + +--- + +## Comparison to Previous Audits + +| Metric | v1.2.1 (Feb 2026) | v2.1.2 (Feb 2026) | v2.4.0 (Apr 2026) | +| --------------- | ----------------- | ----------------- | ------------------------- | +| Critical | 0 | 0 | 0 | +| High | 0 | 0 | 0 | +| Medium | 2 | 0 | 0 | +| Low | 3 | 0 | 4 | +| Enhancements | 4 | 2 | 2 | +| Security Tests | ~20 | 70+ | 70+ (new gaps identified) | +| Overall Posture | GOOD | EXCELLENT | EXCELLENT | + +The four LOW findings represent defence-in-depth gaps and API consistency issues introduced +or exposed by features added since v2.1.2. None involve the core sanitisation or +path-validation mechanisms, which remain sound. + +--- + +## Conclusion + +bash-logger v2.4.0 maintains an **EXCELLENT** security posture. The core protections +established in v2.1.2 — secure-by-default input sanitisation, readonly constant protection, +validated logger path, symlink rejection, and defence-in-depth layering — are intact and +undiminished. + +The four LOW bugs are all correctable in a focused patch release. The two enhancement +requests improve API consistency and audit-trail reliability without touching security +controls. No findings require immediate action from a production-safety perspective. + +**Recommended next action:** Address BUG-04 (error-flag env protection) and BUG-02 +(set_journal_tag validation) as highest-priority, since both directly affect the +integrity of the audit trail. BUG-01 and BUG-03 can follow in the same release. + +--- + +**Audit Status:** ✅ COMPLETE +**Next Review:** Recommended after next major feature addition or 6–12 months +**Report Version:** 1.0 +**Auditor:** Claude (AI Reviewer) — Senior Security Researcher role +**Date:** 2026-04-18 diff --git a/install.sh b/install.sh index 1d55aa4..e7e2a00 100644 --- a/install.sh +++ b/install.sh @@ -561,7 +561,12 @@ download_release() { fi fi elif command -v wget >/dev/null 2>&1; then - # Mirror curl behavior: try IPv4 first, then default settings for both URLs + # wget uses a 4-attempt chain, mirroring curl's IPv4-first preference on both URLs: + # attempt 1: -4 + primary URL (IPv4-forced) + # attempt 2: default + primary URL + # attempt 3: -4 + fallback URL (IPv4-forced; curl omits this step) + # attempt 4: default + fallback URL + # curl uses only 3 attempts (no IPv4-forced retry on the fallback URL). if ! wget -4 --timeout=60 --dns-timeout=10 --connect-timeout=10 \ -qO "${temp_dir}/release.tar.gz" "$download_url" 2>/dev/null; then info "Primary download failed, retrying with default settings..." >&2 diff --git a/logging.sh b/logging.sh index cd8a565..e93b13b 100644 --- a/logging.sh +++ b/logging.sh @@ -66,6 +66,13 @@ if ! readonly -p 2>/dev/null | grep -q "declare -[^ ]*r[^ ]* BASH_LOGGER_VERSION fi done + # Unset deduplication flags that gate write-failure warnings. + # These flags are mutable (legitimately set to "yes" during normal operation) so they + # are not readonly, but they must start empty on each fresh source to prevent a + # pre-existing environment value from permanently suppressing error reporting. + unset LOGGER_FILE_ERROR_REPORTED 2>/dev/null || true + unset LOGGER_JOURNAL_ERROR_REPORTED 2>/dev/null || true + # Log levels (following complete syslog standard - higher number = less severe) # These are readonly to prevent malicious override after initialization readonly LOG_LEVEL_EMERGENCY=0 # System is unusable (most severe) @@ -76,6 +83,7 @@ if ! readonly -p 2>/dev/null | grep -q "declare -[^ ]*r[^ ]* BASH_LOGGER_VERSION readonly LOG_LEVEL_NOTICE=5 # Normal but significant conditions readonly LOG_LEVEL_INFO=6 # Informational messages readonly LOG_LEVEL_DEBUG=7 # Debug information (least severe) + readonly LOG_LEVEL_INIT=6 # Initialization messages (maps to INFO for stderr routing) # Aliases for backward compatibility readonly LOG_LEVEL_FATAL=$LOG_LEVEL_EMERGENCY # Alias for EMERGENCY @@ -235,8 +243,10 @@ if ! readonly -p 2>/dev/null | grep -q "declare -[^ ]*r[^ ]* LOGGER_PATH="; then LOGGER_PATH="" fi # Internal flag: set to "true" by _find_and_validate_logger on every exit path -# (success and failure alike). Allows log_to_journal to skip discovery when -# LOGGER_PATH is empty due to a failed/untrusted lookup rather than no lookup at all. +# (success and failure alike). Used by log_to_journal to skip discovery only when +# LOGGER_PATH is already set — i.e. a previous discovery succeeded and locked the path. +# When LOGGER_PATH is empty (prior discovery failed or logger was not found), discovery +# is always retried so that logger becoming available mid-session is handled correctly. # Not readonly — resetting to "false" on re-source is correct (new context must re-validate). _LOGGER_DISCOVERY_DONE="false" @@ -440,6 +450,32 @@ _validate_config_journal_tag() { return 0 } +# Validate journal tag value (internal) +# Checks for reasonable length and dangerous characters +# Returns 0 if valid, 1 otherwise +_validate_journal_tag() { + local tag="$1" + local max_tag_length=64 + + if [[ -z "$tag" ]]; then + echo "Warning: Empty journal tag" >&2 + return 1 + fi + + if ! _validate_string "$tag" "$max_tag_length" "journal tag" "false" "true"; then + return 1 + fi + + # Check for shell metacharacters that could cause issues + # Character class includes: $ ` ; | & < > ( ) { } [ ] \ + if [[ "$tag" =~ []$\`\;\|\&\<\>\(\)\{\}\[\\] ]]; then + echo "Warning: Journal tag contains shell metacharacters" >&2 + return 1 + fi + + return 0 +} + # Validate syslog facility value (internal) # Returns 0 if valid, 1 otherwise _validate_syslog_facility() { @@ -460,6 +496,18 @@ _validate_syslog_facility() { esac } +# Parse a boolean string value to a canonical "true" or "false" (internal) +# Accepts: true/yes/on/1 -> prints "true"; false/no/off/0 -> prints "false" +# Returns 0 on success, 1 for unrecognised input (prints nothing) +_parse_bool_value() { + local input="${1,,}" + case "$input" in + true|yes|on|1) echo "true"; return 0 ;; + false|no|off|0) echo "false"; return 0 ;; + *) return 1 ;; + esac +} + # Parse an INI-style configuration file (internal) # Usage: _parse_config_file "/path/to/config.ini" # Returns 0 on success, 1 on error @@ -663,30 +711,14 @@ _parse_config_file() { esac ;; unsafe_allow_newlines|unsafe-allow-newlines) - case "${value,,}" in - true|yes|1|on) - LOG_UNSAFE_ALLOW_NEWLINES="true" - ;; - false|no|0|off) - LOG_UNSAFE_ALLOW_NEWLINES="false" - ;; - *) - echo "Warning: Invalid unsafe_allow_newlines value '$value' at line $line_num, expected true/false" >&2 - ;; - esac + if ! LOG_UNSAFE_ALLOW_NEWLINES=$(_parse_bool_value "$value"); then + echo "Warning: Invalid unsafe_allow_newlines value '$value' at line $line_num, expected true/false" >&2 + fi ;; unsafe_allow_ansi_codes|unsafe-allow-ansi-codes) - case "${value,,}" in - true|yes|1|on) - LOG_UNSAFE_ALLOW_ANSI_CODES="true" - ;; - false|no|0|off) - LOG_UNSAFE_ALLOW_ANSI_CODES="false" - ;; - *) - echo "Warning: Invalid unsafe_allow_ansi_codes value '$value' at line $line_num, expected true/false" >&2 - ;; - esac + if ! LOG_UNSAFE_ALLOW_ANSI_CODES=$(_parse_bool_value "$value"); then + echo "Warning: Invalid unsafe_allow_ansi_codes value '$value' at line $line_num, expected true/false" >&2 + fi ;; max_line_length|max-line-length|log_max_line_length|log-max-line-length) if [[ "$value" =~ ^[0-9]+$ ]] && [[ "$value" -ge 0 ]] && [[ "$value" -le 1048576 ]]; then @@ -965,10 +997,24 @@ _strip_ansi_codes() { local step2b step2b=$(printf '%s' "$step2" | sed "s|${esc}][^${esc}]*${esc}\\\\||g") + # Remove DCS (Device Control String) sequences: ESC P ... ESC \ + # These carry device control payloads (e.g. Sixel graphics, DECRQSS responses). + # Both the payload and the ST terminator must be removed along with the opener. + local step2c + step2c=$(printf '%s' "$step2b" | sed "s|${esc}P[^${esc}]*${esc}\\\\||g") + + # Remove PM (Privacy Message) sequences: ESC ^ ... ESC \ + local step2d + step2d=$(printf '%s' "$step2c" | sed "s|${esc}^[^${esc}]*${esc}\\\\||g") + + # Remove APC (Application Program Command) sequences: ESC _ ... ESC \ + local step2e + step2e=$(printf '%s' "$step2d" | sed "s|${esc}_[^${esc}]*${esc}\\\\||g") + # Remove remaining escape sequences (simplified fallback) local step3 # shellcheck disable=SC1117 - step3=$(printf '%s' "$step2b" | sed 's/\x1b[^[]//g') + step3=$(printf '%s' "$step2e" | sed 's/\x1b[^[]//g') echo "$step3" } @@ -1453,6 +1499,10 @@ set_journal_logging() { # Function to set journal tag set_journal_tag() { + if ! _validate_journal_tag "$1"; then + return 1 + fi + local old_tag="$JOURNAL_TAG" JOURNAL_TAG="$1" @@ -1514,22 +1564,27 @@ set_syslog_facility() { set_color_mode() { local mode="$1" local old_setting="$USE_COLORS" + local new_value case "$mode" in true|on|yes|1) - USE_COLORS="always" + new_value="always" ;; false|off|no|0) - USE_COLORS="never" + new_value="never" ;; - auto) - USE_COLORS="auto" + auto|always|never) + new_value="$mode" ;; *) - USE_COLORS="$mode" # Set directly if it's already "always", "never", or "auto" + echo "Error: set_color_mode: unrecognised mode '$mode'" >&2 + echo " Valid modes: auto, always, never (or: true/false, on/off, yes/no, 1/0)" >&2 + return 1 ;; esac + USE_COLORS="$new_value" + local message="Color mode changed from \"$old_setting\" to \"$USE_COLORS\"" local log_entry log_entry=$(_format_log_message "CONFIG" "$message") @@ -1584,8 +1639,14 @@ set_script_name() { # WARNING: Disabling sanitization can allow log injection attacks. Only use if you have # explicit control over all logged messages and your log parsing handles newlines safely. set_unsafe_allow_newlines() { + local new_value + new_value=$(_parse_bool_value "$1") || { + echo "Error: set_unsafe_allow_newlines: invalid value '$1'" >&2 + echo " Valid values: true, false, yes, no, on, off, 1, 0" >&2 + return 1 + } local old_setting="$LOG_UNSAFE_ALLOW_NEWLINES" - LOG_UNSAFE_ALLOW_NEWLINES="$1" + LOG_UNSAFE_ALLOW_NEWLINES="$new_value" local safety_notice="" if [[ "$LOG_UNSAFE_ALLOW_NEWLINES" == "true" ]]; then @@ -1627,8 +1688,14 @@ set_unsafe_allow_newlines() { # WARNING: Disabling sanitization can allow terminal manipulation attacks. Only use if you have # explicit control over all logged messages and trust their source. set_unsafe_allow_ansi_codes() { + local new_value + new_value=$(_parse_bool_value "$1") || { + echo "Error: set_unsafe_allow_ansi_codes: invalid value '$1'" >&2 + echo " Valid values: true, false, yes, no, on, off, 1, 0" >&2 + return 1 + } local old_setting="$LOG_UNSAFE_ALLOW_ANSI_CODES" - LOG_UNSAFE_ALLOW_ANSI_CODES="$1" + LOG_UNSAFE_ALLOW_ANSI_CODES="$new_value" local safety_notice="" if [[ "$LOG_UNSAFE_ALLOW_ANSI_CODES" == "true" ]]; then @@ -1700,10 +1767,12 @@ _log_message() { local skip_file="${4:-false}" local skip_journal="${5:-false}" local force_journal="${6:-false}" + local force_show="${7:-false}" # Skip logging if message level is more verbose than current log level # With syslog-style levels, HIGHER values are LESS severe (more verbose) - if [[ "$level_value" -gt "$CURRENT_LOG_LEVEL" ]]; then + # force_show=true bypasses this filter (used by log_init to always show) + if [[ "$level_value" -gt "$CURRENT_LOG_LEVEL" && "$force_show" != "true" ]]; then return fi @@ -1765,49 +1834,49 @@ _log_message() { # Helper functions for different log levels log_debug() { - _log_message "DEBUG" $LOG_LEVEL_DEBUG "$1" + _log_message "DEBUG" "$LOG_LEVEL_DEBUG" "$1" } log_info() { - _log_message "INFO" $LOG_LEVEL_INFO "$1" + _log_message "INFO" "$LOG_LEVEL_INFO" "$1" } log_notice() { - _log_message "NOTICE" $LOG_LEVEL_NOTICE "$1" + _log_message "NOTICE" "$LOG_LEVEL_NOTICE" "$1" } log_warn() { - _log_message "WARN" $LOG_LEVEL_WARN "$1" + _log_message "WARN" "$LOG_LEVEL_WARN" "$1" } log_error() { - _log_message "ERROR" $LOG_LEVEL_ERROR "$1" + _log_message "ERROR" "$LOG_LEVEL_ERROR" "$1" } log_critical() { - _log_message "CRITICAL" $LOG_LEVEL_CRITICAL "$1" + _log_message "CRITICAL" "$LOG_LEVEL_CRITICAL" "$1" } log_alert() { - _log_message "ALERT" $LOG_LEVEL_ALERT "$1" + _log_message "ALERT" "$LOG_LEVEL_ALERT" "$1" } log_emergency() { - _log_message "EMERGENCY" $LOG_LEVEL_EMERGENCY "$1" + _log_message "EMERGENCY" "$LOG_LEVEL_EMERGENCY" "$1" } # Alias for backward compatibility log_fatal() { - _log_message "FATAL" $LOG_LEVEL_EMERGENCY "$1" + _log_message "FATAL" "$LOG_LEVEL_EMERGENCY" "$1" } log_init() { - _log_message "INIT" -1 "$1" # Using -1 to ensure it always shows + _log_message "INIT" "$LOG_LEVEL_INIT" "$1" "false" "false" "false" "true" } # Function for sensitive logging - console only, never to file or journal log_sensitive() { - _log_message "SENSITIVE" $LOG_LEVEL_INFO "$1" "true" "true" + _log_message "SENSITIVE" "$LOG_LEVEL_INFO" "$1" "true" "true" } # Log a single message directly to the system journal, regardless of USE_JOURNAL state. @@ -1864,11 +1933,11 @@ log_to_journal() { return 0 fi - # Fast-path: if discovery has already run (successfully or not), skip it. - # _LOGGER_DISCOVERY_DONE is set on every exit path of _find_and_validate_logger, - # so an empty LOGGER_PATH with the flag set means logger is absent or untrusted — - # no point repeating command -v, symlink resolution, and path validation. - if [[ "$_LOGGER_DISCOVERY_DONE" != "true" ]]; then + # Fast-path: skip discovery only when LOGGER_PATH is already set (a prior successful + # discovery locked the path as readonly). If LOGGER_PATH is empty — regardless of + # whether _LOGGER_DISCOVERY_DONE is true — retry discovery so that logger becoming + # available mid-session (installed, PATH corrected, etc.) is handled correctly. + if [[ "$_LOGGER_DISCOVERY_DONE" != "true" || -z "$LOGGER_PATH" ]]; then check_logger_available fi diff --git a/scripts/create-issues.sh b/scripts/create-issues.sh new file mode 100644 index 0000000..277cc5c --- /dev/null +++ b/scripts/create-issues.sh @@ -0,0 +1,222 @@ +#!/usr/bin/env bash +# +# create-issues.sh - Create GitHub issues from markdown issue files +# +# Reads YAML frontmatter (title, labels) from each markdown file and creates +# the corresponding GitHub issue using the gh CLI. The frontmatter block is +# stripped so only the issue body is submitted. +# +# Usage: +# ./create-issues.sh [--dry-run] [--repo OWNER/REPO] [FILE...] +# +# If no files are given, all *.md files in the current directory are processed. +# +# Requirements: +# - gh CLI installed and authenticated (gh auth login) +# - Write access to the target repository +# +# Examples: +# ./create-issues.sh # all *.md in current dir +# ./create-issues.sh BUG-*.md FEATURE-*.md DOCS-*.md # specific files +# ./create-issues.sh --dry-run *.md # preview without creating +# ./create-issues.sh --repo GingerGraham/bash-logger BUG-01-*.md + +set -euo pipefail + +# ── Defaults ────────────────────────────────────────────────────────────────── +DRY_RUN="false" +REPO="" # empty = gh infers from current git remote +CREATED=0 +SKIPPED=0 +FAILED=0 + +# ── Helpers ─────────────────────────────────────────────────────────────────── +usage() { + grep '^#' "$0" | grep -v '#!/' | sed 's/^# \{0,1\}//' + exit 0 +} + +log() { printf '\e[34m[INFO]\e[0m %s\n' "$*" >&2; } +ok() { printf '\e[32m[OK]\e[0m %s\n' "$*" >&2; } +warn() { printf '\e[33m[WARN]\e[0m %s\n' "$*" >&2; } +err() { printf '\e[31m[ERROR]\e[0m %s\n' "$*" >&2; } + +# Extract a scalar value from YAML frontmatter: extract_fm_value "title" +extract_fm_value() { + local key="$1" + local file="$2" + # Match key: value between the opening and closing --- delimiters + awk -v key="$key" ' + /^---$/ { if (in_fm) exit; in_fm=1; next } + in_fm && $0 ~ "^"key":" { + sub("^"key":[[:space:]]*", "") + # Strip surrounding quotes if present + gsub(/^["\x27]|["\x27]$/, "") + print + exit + } + ' "$file" +} + +# Extract a list value from YAML frontmatter: extract_fm_list "labels" +# Returns comma-separated values (gh --label accepts comma-separated string) +extract_fm_list() { + local key="$1" + local file="$2" + awk -v key="$key" ' + /^---$/ { if (in_fm) exit; in_fm=1; next } + in_fm && found { + # List item line + if (/^[[:space:]]+-[[:space:]]/) { + sub(/^[[:space:]]+-[[:space:]]/, "") + gsub(/^["\x27]|["\x27]$/, "") + items = (items ? items "," : "") $0 + next + } + # Non-list line after key — stop + exit + } + in_fm && $0 ~ "^"key":" { + val = $0 + sub("^"key":[[:space:]]*", "", val) + gsub(/^["\x27]|["\x27]$/, "", val) + if (val != "") { + # Inline value (e.g. labels: bug) + items = val + exit + } + found = 1 + } + END { print items } + ' "$file" +} + +# Strip YAML frontmatter and return only the body content +strip_frontmatter() { + local file="$1" + awk ' + /^---$/ { + if (!in_fm && !past_fm) { in_fm=1; next } + if (in_fm) { in_fm=0; past_fm=1; next } + } + past_fm { print } + ' "$file" +} + +# ── Argument parsing ────────────────────────────────────────────────────────── +FILES=() +while [[ $# -gt 0 ]]; do + case "$1" in + --dry-run|-n) DRY_RUN="true"; shift ;; + --repo|-r) + if [[ $# -lt 2 ]]; then + err "Option $1 requires an argument." + exit 1 + fi + REPO="$2" + shift 2 + ;; + --help|-h) usage ;; + --) shift; FILES+=("$@"); break ;; + -*) err "Unknown option: $1"; exit 1 ;; + *) FILES+=("$1"); shift ;; + esac +done + +# Default to all *.md in current directory if no files specified +if [[ ${#FILES[@]} -eq 0 ]]; then + while IFS= read -r f; do FILES+=("$f"); done < <(find . -maxdepth 1 -name '*.md' | sort) +fi + +if [[ ${#FILES[@]} -eq 0 ]]; then + err "No markdown files found." + exit 1 +fi + +# ── Preflight checks ────────────────────────────────────────────────────────── +if ! command -v gh >/dev/null 2>&1; then + err "gh CLI not found. Install it using your system package manager or see:" + err "https://cli.github.com/" + err "Then authenticate with: gh auth login" + exit 1 +fi + +if [[ "$DRY_RUN" == "false" ]] && ! gh auth status >/dev/null 2>&1; then + err "Not authenticated with gh. Run: gh auth login" + exit 1 +fi + +if [[ "$DRY_RUN" == "true" ]]; then + warn "Dry-run mode — no issues will be created" +fi + +# ── Process files ───────────────────────────────────────────────────────────── +TMPBODY=$(mktemp /tmp/issue-body-XXXXXX.md) +trap 'rm -f "$TMPBODY"' EXIT + +for file in "${FILES[@]}"; do + if [[ ! -f "$file" ]]; then + warn "File not found, skipping: $file" + ((SKIPPED++)) || true + continue + fi + + log "Processing: $file" + + # Parse frontmatter + title=$(extract_fm_value "title" "$file") + labels=$(extract_fm_list "labels" "$file") + + if [[ -z "$title" ]]; then + warn "No 'title' found in frontmatter of $file — skipping" + ((SKIPPED++)) || true + continue + fi + + # Write body (frontmatter stripped) to temp file + strip_frontmatter "$file" > "$TMPBODY" + + # Build gh command + gh_args=(issue create --title "$title" --body-file "$TMPBODY") + + if [[ -n "$labels" ]]; then + # gh accepts comma-separated labels; split into multiple --label flags for safety + IFS=',' read -ra label_arr <<< "$labels" + for lbl in "${label_arr[@]}"; do + lbl="${lbl#"${lbl%%[![:space:]]*}"}" # trim leading whitespace + lbl="${lbl%"${lbl##*[![:space:]]}"}" # trim trailing whitespace + [[ -n "$lbl" ]] && gh_args+=(--label "$lbl") + done + fi + + if [[ -n "$REPO" ]]; then + gh_args+=(--repo "$REPO") + fi + + if [[ "$DRY_RUN" == "true" ]]; then + warn " [DRY RUN] Would run: gh ${gh_args[*]}" + warn " [DRY RUN] Title: $title" + warn " [DRY RUN] Labels: ${labels:-none}" + ((CREATED++)) || true + continue + fi + + # Create the issue + if issue_url=$(gh "${gh_args[@]}" 2>&1); then + ok " Created: $issue_url" + ((CREATED++)) || true + else + err " Failed to create issue for: $file" + err " gh output: $issue_url" + ((FAILED++)) || true + fi +done + +# ── Summary ─────────────────────────────────────────────────────────────────── +echo "" +printf '\e[1mSummary\e[0m\n' +printf ' %s %s\n' "$(printf '\e[32m✓\e[0m')" "Created: $CREATED" +printf ' %s %s\n' "$(printf '\e[33m-\e[0m')" "Skipped: $SKIPPED" +printf ' %s %s\n' "$(printf '\e[31m✗\e[0m')" "Failed: $FAILED" + +[[ "$FAILED" -gt 0 ]] && exit 1 || exit 0 diff --git a/tests/test_ansi_injection.sh b/tests/test_ansi_injection.sh index 7f98e79..e5a2d2d 100755 --- a/tests/test_ansi_injection.sh +++ b/tests/test_ansi_injection.sh @@ -381,6 +381,81 @@ test_other_csi_parameter_bytes_stripped() { fi } +# Test: DCS sequences are fully stripped (payload + ST terminator) +test_dcs_sequences_stripped() { + start_test "DCS sequences are fully stripped including payload and ST terminator" + + # DCS sequence: ESC P ESC \ (e.g. Sixel graphics, DECRQSS responses) + local malicious_input=$'\ePsixel-data-here\e\\' + local expected="" + + LOG_UNSAFE_ALLOW_ANSI_CODES="false" + local sanitized + sanitized=$(_strip_ansi_codes "$malicious_input") + + if [[ "$sanitized" == "$expected" ]]; then + pass_test + else + fail_test "DCS payload not stripped: got '$sanitized' expected ''" + fi +} + +# Test: PM sequences are fully stripped (payload + ST terminator) +test_pm_sequences_stripped() { + start_test "PM sequences are fully stripped including payload and ST terminator" + + # PM sequence: ESC ^ ESC \ (Privacy Message) + local malicious_input=$'\e^metadata\e\\' + local expected="" + + LOG_UNSAFE_ALLOW_ANSI_CODES="false" + local sanitized + sanitized=$(_strip_ansi_codes "$malicious_input") + + if [[ "$sanitized" == "$expected" ]]; then + pass_test + else + fail_test "PM payload not stripped: got '$sanitized' expected ''" + fi +} + +# Test: APC sequences are fully stripped (payload + ST terminator) +test_apc_sequences_stripped() { + start_test "APC sequences are fully stripped including payload and ST terminator" + + # APC sequence: ESC _ ESC \ (Application Program Command) + local malicious_input=$'\e_app-data\e\\' + local expected="" + + LOG_UNSAFE_ALLOW_ANSI_CODES="false" + local sanitized + sanitized=$(_strip_ansi_codes "$malicious_input") + + if [[ "$sanitized" == "$expected" ]]; then + pass_test + else + fail_test "APC payload not stripped: got '$sanitized' expected ''" + fi +} + +# Test: Mixed DCS, PM, and APC sequences interleaved with normal text are stripped +test_mixed_dcs_pm_apc_stripped() { + start_test "Mixed DCS, PM, and APC sequences interleaved with normal text are stripped" + + local malicious_input=$'Start\ePdcs-payload\e\\Middle\e^pm-payload\e\\End\e_apc-payload\e\\Tail' + local expected="StartMiddleEndTail" + + LOG_UNSAFE_ALLOW_ANSI_CODES="false" + local sanitized + sanitized=$(_strip_ansi_codes "$malicious_input") + + if [[ "$sanitized" == "$expected" ]]; then + pass_test + else + fail_test "Mixed DCS/PM/APC not stripped: got '$sanitized' expected '$expected'" + fi +} + # Run all tests test_default_strips_csi_sequences test_default_strips_screen_control @@ -401,6 +476,10 @@ test_osc_with_embedded_escapes_stripped test_multiple_consecutive_osc_stripped test_mixed_mode_newlines_allowed_ansi_stripped test_other_csi_parameter_bytes_stripped +test_dcs_sequences_stripped +test_pm_sequences_stripped +test_apc_sequences_stripped +test_mixed_dcs_pm_apc_stripped # Cleanup after running all tests cleanup_test_suite diff --git a/tests/test_environment_security.sh b/tests/test_environment_security.sh index a3423e2..48402b5 100755 --- a/tests/test_environment_security.sh +++ b/tests/test_environment_security.sh @@ -497,6 +497,54 @@ test_readonly_variable_conflicts() { fi } +# Test: Pre-set LOGGER_FILE_ERROR_REPORTED is cleared at source time +test_preset_file_error_reported_flag_cleared() { + start_test "Pre-set LOGGER_FILE_ERROR_REPORTED is cleared at source time" + + local log_file="$TEST_DIR/flagtest.log" + local stderr_file="$TEST_DIR/flagtest.stderr" + + # Export the flag before sourcing to simulate a pre-injected environment value + export LOGGER_FILE_ERROR_REPORTED="injected" + + bash -c " + source '$PROJECT_ROOT/logging.sh' + init_logger --log '$log_file' --no-color --quiet + touch '$log_file' + chmod 000 '$log_file' + log_info 'trigger write failure' + " 2>"$stderr_file" + + unset LOGGER_FILE_ERROR_REPORTED + + # The warning must appear — confirming the flag was cleared on source and not suppressed + assert_file_contains "$stderr_file" "Failed to write to log file" || return + + pass_test +} + +# Test: Pre-set LOGGER_JOURNAL_ERROR_REPORTED is cleared at source time +test_preset_journal_error_reported_flag_cleared() { + start_test "Pre-set LOGGER_JOURNAL_ERROR_REPORTED is cleared at source time" + + local state_file="$TEST_DIR/journal_flag.state" + + # Export the flag before sourcing to simulate a pre-injected environment value + export LOGGER_JOURNAL_ERROR_REPORTED="injected" + + bash -c " + source '$PROJECT_ROOT/logging.sh' + echo \"LOGGER_JOURNAL_ERROR_REPORTED=\${LOGGER_JOURNAL_ERROR_REPORTED:-}\" > '$state_file' + " + + unset LOGGER_JOURNAL_ERROR_REPORTED + + # The flag must be empty after sourcing — confirming it was cleared by the unset + assert_file_not_contains "$state_file" "injected" || return + + pass_test +} + # Run all tests test_preset_log_file_handling test_malicious_path_variable @@ -516,3 +564,5 @@ test_cdpath_variable test_globignore_variable test_prompt_command_injection test_readonly_variable_conflicts +test_preset_file_error_reported_flag_cleared +test_preset_journal_error_reported_flag_cleared diff --git a/tests/test_install.sh b/tests/test_install.sh index 08ee026..f648a1e 100755 --- a/tests/test_install.sh +++ b/tests/test_install.sh @@ -15,7 +15,7 @@ run_install_test_script() { local script_content="$1" local test_script="$TEST_DIR/test_script_$$.sh" - + cat > "$test_script" << EOF #!/usr/bin/env bash set -euo pipefail @@ -25,7 +25,7 @@ main() { :; } $script_content EOF - + bash "$test_script" 2>&1 } @@ -33,7 +33,7 @@ EOF assert_command_failed() { local exit_code="$1" local message="$2" - + if [[ ${exit_code:-0} -eq 0 ]]; then fail_test "$message" return 1 @@ -1345,15 +1345,17 @@ download_release "0.10.0" "$TEMP_DIR" pass_test } -# Test: download_release falls back to tag archive when primary asset download fails -test_download_release_fallback_on_primary_failure() { - start_test "download_release falls back to tag archive URL when primary asset download fails" +# Test: download_release (curl) falls back to tag archive when primary asset download fails +# curl uses a 3-attempt chain: IPv4+primary -> default+primary -> default+fallback +test_download_release_curl_fallback_on_primary_failure() { + start_test "download_release (curl) falls back to tag archive URL when primary asset download fails" local output output=$(run_install_test_script ' TEMP_DIR="${TEST_TMP_DIR}/fallback_test" mkdir -p "$TEMP_DIR" +# Fail attempts 1-2 (IPv4+primary, default+primary); succeed on attempt 3 (default+fallback) MOCK_CURL_CALLS=0 curl() { MOCK_CURL_CALLS=$((MOCK_CURL_CALLS + 1)) @@ -1376,6 +1378,37 @@ curl() { return 0 } +download_release "0.10.0" "$TEMP_DIR" +') + + assert_contains "$output" "falling back to tag archive" || return + assert_contains "$output" "archive/refs/tags/0.10.0" || return + assert_contains "$output" "CURL_CALL_3" || return + + pass_test +} + +# Test: download_release (wget) falls back to tag archive when primary asset download fails +# wget uses a 4-attempt chain: IPv4+primary -> default+primary -> IPv4+fallback -> default+fallback +# This differs from curl (3 attempts): wget adds an IPv4-forced retry on the fallback URL too. +test_download_release_wget_fallback_on_primary_failure() { + start_test "download_release (wget) falls back to tag archive URL when primary asset download fails" + + local output + output=$(run_install_test_script ' +TEMP_DIR="${TEST_TMP_DIR}/wget_fallback_test" +mkdir -p "$TEMP_DIR" + +# Override command() to hide curl so the wget code path in download_release is exercised +command() { + case "$1 $2" in + "-v curl") return 1 ;; + "-v wget") return 0 ;; + *) builtin command "$@" ;; + esac +} + +# Fail attempts 1-3 (IPv4+primary, default+primary, IPv4+fallback); succeed on attempt 4 (default+fallback) MOCK_WGET_CALLS=0 wget() { MOCK_WGET_CALLS=$((MOCK_WGET_CALLS + 1)) @@ -1386,7 +1419,7 @@ wget() { prev_arg="$arg" done echo "WGET_CALL_${MOCK_WGET_CALLS}: $url" - if [[ $MOCK_WGET_CALLS -le 2 ]]; then + if [[ $MOCK_WGET_CALLS -le 3 ]]; then return 1 fi if [[ -n "$output_file" ]]; then @@ -1403,6 +1436,7 @@ download_release "0.10.0" "$TEMP_DIR" assert_contains "$output" "falling back to tag archive" || return assert_contains "$output" "archive/refs/tags/0.10.0" || return + assert_contains "$output" "WGET_CALL_4" || return pass_test } @@ -1479,5 +1513,6 @@ test_configure_rc_file_already_installed_with_auto_rc test_configure_rc_file_already_installed_without_auto_rc test_configure_rc_file_already_installed_system_mode test_download_release_url_construction -test_download_release_fallback_on_primary_failure +test_download_release_curl_fallback_on_primary_failure +test_download_release_wget_fallback_on_primary_failure test_download_release_error_on_all_failures diff --git a/tests/test_journal_logging.sh b/tests/test_journal_logging.sh index 39d847a..27d0b44 100755 --- a/tests/test_journal_logging.sh +++ b/tests/test_journal_logging.sh @@ -468,6 +468,98 @@ test_no_journal_via_runtime() { pass_test } +# --------------------------------------------------------------------------- +# Test 15: log_to_journal retries discovery after a failed init_logger --journal +# Regression test for https://github.com/GingerGraham/bash-logger/issues/93 +# Reproduces the exact scenario: _LOGGER_DISCOVERY_DONE=true + LOGGER_PATH="" +# (the state left by a failed init_logger --journal), then a stub logger appears. +# --------------------------------------------------------------------------- +test_log_to_journal_retries_discovery_after_failed_init() { + start_test "log_to_journal retries discovery when LOGGER_PATH empty after failed init (issue #93)" + + _create_stub_logger "$TEST_DIR" + + local result exit_code + result=$(bash -c " + source '$PROJECT_ROOT/logging.sh' + + # Simulate the state left by a failed init_logger --journal: + # discovery was attempted (_LOGGER_DISCOVERY_DONE=true) but logger was not + # found or not trusted (LOGGER_PATH=''). + # Override _find_and_validate_logger to reflect that the stub is now available. + _find_and_validate_logger() { + LOGGER_PATH='$STUB_LOGGER' + _LOGGER_DISCOVERY_DONE='true' + return 0 + } + check_logger_available() { _find_and_validate_logger; } + + _LOGGER_DISCOVERY_DONE='true' + LOGGER_PATH='' + USE_JOURNAL='false' + + init_logger --no-color --quiet 2>/dev/null + + log_to_journal INFO 'issue93_rediscovery_marker' + echo exit:\$? + " 2>&1) + exit_code=$(echo "$result" | grep -o 'exit:[0-9]*' | cut -d: -f2) + + assert_equals "0" "$exit_code" \ + "log_to_journal should return 0 after successful rediscovery" || return + assert_file_contains "$STUB_CAPTURE" "issue93_rediscovery_marker" \ + "Stub logger should have captured the message after rediscovery" || return + + pass_test +} + +# --------------------------------------------------------------------------- +# Test 16: log_to_journal retries discovery on second call after first failure +# Even after a warning is emitted on the first call (logger unavailable), the +# second call should still attempt re-discovery when LOGGER_PATH is still empty. +# --------------------------------------------------------------------------- +test_log_to_journal_retries_on_second_call_after_first_failure() { + start_test "log_to_journal retries discovery on second call after first failure" + + _create_stub_logger "$TEST_DIR" + + local result exit_code + result=$(bash -c " + source '$PROJECT_ROOT/logging.sh' + + # First call: logger not available — discovery fails, warning emitted. + # Second call: stub logger now available — discovery should be retried. + _attempt=0 + _find_and_validate_logger() { + _attempt=\$(( _attempt + 1 )) + if [[ \$_attempt -lt 2 ]]; then + LOGGER_PATH='' + _LOGGER_DISCOVERY_DONE='true' + return 1 + fi + LOGGER_PATH='$STUB_LOGGER' + _LOGGER_DISCOVERY_DONE='true' + return 0 + } + check_logger_available() { _find_and_validate_logger; } + + init_logger --no-color --quiet 2>/dev/null + USE_JOURNAL='false' + + log_to_journal INFO 'first_call_should_fail' 2>/dev/null || true + log_to_journal INFO 'second_call_should_succeed' + echo exit:\$? + " 2>&1) + exit_code=$(echo "$result" | grep -o 'exit:[0-9]*' | cut -d: -f2) + + assert_equals "0" "$exit_code" \ + "log_to_journal second call should return 0 after rediscovery" || return + assert_file_contains "$STUB_CAPTURE" "second_call_should_succeed" \ + "Stub logger should have captured the second message after rediscovery" || return + + pass_test +} + # Run all tests test_journal_option test_no_journal_via_runtime @@ -483,3 +575,5 @@ test_log_to_journal_no_double_warn_when_journal_enabled test_log_sensitive_unaffected_by_force_journal test_log_to_journal_level_aliases test_log_to_journal_numeric_levels +test_log_to_journal_retries_discovery_after_failed_init +test_log_to_journal_retries_on_second_call_after_first_failure diff --git a/tests/test_output.sh b/tests/test_output.sh index 090c9e4..d7c9d5e 100755 --- a/tests/test_output.sh +++ b/tests/test_output.sh @@ -402,6 +402,54 @@ test_set_color_mode_auto() { pass_test } +# Test: log_init respects --stderr-level EMERGENCY (strict threshold) +test_log_init_respects_stderr_level_emergency() { + start_test "log_init does not route to stderr when --stderr-level EMERGENCY" + + bash -c " + source '$PROJECT_ROOT/logging.sh' + init_logger --stderr-level EMERGENCY --no-color + log_init 'Application starting' + " >"$TEST_DIR/stdout" 2>"$TEST_DIR/stderr" + + assert_file_contains "$TEST_DIR/stdout" "Application starting" || return + assert_file_not_contains "$TEST_DIR/stderr" "Application starting" || return + + pass_test +} + +# Test: log_init routes to stderr when --stderr-level DEBUG (permissive threshold) +test_log_init_respects_stderr_level_debug() { + start_test "log_init routes to stderr when --stderr-level DEBUG" + + bash -c " + source '$PROJECT_ROOT/logging.sh' + init_logger --stderr-level DEBUG --no-color + log_init 'Application starting' + " >"$TEST_DIR/stdout" 2>"$TEST_DIR/stderr" + + assert_file_contains "$TEST_DIR/stderr" "Application starting" || return + + pass_test +} + +# Test: log_init always shows even when --level is more restrictive +test_log_init_always_shows_regardless_of_level() { + start_test "log_init always shows regardless of log level filter" + + bash -c " + source '$PROJECT_ROOT/logging.sh' + init_logger --level ERROR --no-color + log_init 'Application starting' + " >"$TEST_DIR/stdout" 2>"$TEST_DIR/stderr" + + local combined + combined=$(cat "$TEST_DIR/stdout" "$TEST_DIR/stderr") + assert_contains "$combined" "Application starting" || return + + pass_test +} + # Run all tests test_console_output_default test_console_output_quiet @@ -424,3 +472,6 @@ test_log_line_truncation test_set_color_mode_always test_set_color_mode_never test_set_color_mode_auto +test_log_init_respects_stderr_level_emergency +test_log_init_respects_stderr_level_debug +test_log_init_always_shows_regardless_of_level diff --git a/tests/test_runtime_config.sh b/tests/test_runtime_config.sh index 0f57bfb..6501cf3 100755 --- a/tests/test_runtime_config.sh +++ b/tests/test_runtime_config.sh @@ -192,6 +192,92 @@ test_set_journal_tag() { pass_test } +# Test: set_journal_tag rejects empty string +test_set_journal_tag_rejects_empty() { + start_test "set_journal_tag rejects empty tag" + + init_logger + set_journal_tag "original-tag" + + if set_journal_tag "" >/dev/null 2>&1; then + fail_test "set_journal_tag should return non-zero for empty tag" + return + fi + + assert_equals "original-tag" "$JOURNAL_TAG" || return + + pass_test +} + +# Test: set_journal_tag rejects oversized tags +test_set_journal_tag_rejects_long_tag() { + start_test "set_journal_tag rejects tag longer than 64 characters" + + init_logger + set_journal_tag "short-tag" + + local long_tag + long_tag=$(printf 'A%.0s' {1..200}) + + if set_journal_tag "$long_tag" >/dev/null 2>&1; then + fail_test "set_journal_tag should return non-zero for tag exceeding 64 characters" + return + fi + + assert_equals "short-tag" "$JOURNAL_TAG" || return + + pass_test +} + +# Test: set_journal_tag rejects tags with shell metacharacters +test_set_journal_tag_rejects_metacharacters() { + start_test "set_journal_tag rejects tag containing shell metacharacters" + + init_logger + set_journal_tag "safe-tag" + + if set_journal_tag 'bad$tag' >/dev/null 2>&1; then + fail_test "set_journal_tag should return non-zero for tag with metacharacters" + return + fi + + assert_equals "safe-tag" "$JOURNAL_TAG" || return + + pass_test +} + +# Test: set_journal_tag rejects tags with control characters +test_set_journal_tag_rejects_control_chars() { + start_test "set_journal_tag rejects tag containing control characters" + + init_logger + set_journal_tag "clean-tag" + + if set_journal_tag $'tag\twith\ttabs' >/dev/null 2>&1; then + fail_test "set_journal_tag should return non-zero for tag with control characters" + return + fi + + assert_equals "clean-tag" "$JOURNAL_TAG" || return + + pass_test +} + +# Test: set_journal_tag preserves existing tag on rejection +test_set_journal_tag_preserves_tag_on_rejection() { + start_test "set_journal_tag preserves existing tag when new value is invalid" + + init_logger + set_journal_tag "my-app" + assert_equals "my-app" "$JOURNAL_TAG" || return + + set_journal_tag "$(printf 'X%.0s' {1..200})" >/dev/null 2>&1 || true + + assert_equals "my-app" "$JOURNAL_TAG" || return + + pass_test +} + # Test: set_syslog_facility with valid value test_set_syslog_facility_valid() { start_test "set_syslog_facility accepts valid facility" @@ -254,6 +340,54 @@ test_set_color_mode() { pass_test } +# Test: set_color_mode rejects unrecognised mode +test_set_color_mode_rejects_invalid() { + start_test "set_color_mode rejects unrecognised mode" + + init_logger + + if set_color_mode "foobar" >/dev/null 2>&1; then + fail_test "set_color_mode should return non-zero for unrecognised mode 'foobar'" + return + fi + + pass_test +} + +# Test: set_color_mode preserves USE_COLORS on invalid input +test_set_color_mode_preserves_state_on_invalid() { + start_test "set_color_mode preserves USE_COLORS on invalid input" + + init_logger + set_color_mode "always" + + if set_color_mode "foobar" >/dev/null 2>&1; then + fail_test "set_color_mode should return non-zero for unrecognised mode 'foobar'" + return + fi + + assert_equals "always" "$USE_COLORS" || return + + pass_test +} + +# Test: set_color_mode rejects wrong-case canonical value +test_set_color_mode_rejects_wrong_case() { + start_test "set_color_mode rejects wrong-case canonical value" + + init_logger + set_color_mode "auto" + + if set_color_mode "ALWAYS" >/dev/null 2>&1; then + fail_test "set_color_mode should return non-zero for wrong-case value 'ALWAYS'" + return + fi + + assert_equals "auto" "$USE_COLORS" || return + + pass_test +} + # Test: Multiple runtime changes test_multiple_runtime_changes() { start_test "Multiple runtime changes work together" @@ -577,6 +711,50 @@ test_set_unsafe_allow_newlines() { pass_test } +# Test: set_unsafe_allow_newlines normalises truthy inputs +test_set_unsafe_allow_newlines_truthy_variants() { + start_test "set_unsafe_allow_newlines accepts truthy variants" + + init_logger --quiet + + for val in yes on 1 YES On YES; do + set_unsafe_allow_newlines "$val" + assert_equals "true" "$LOG_UNSAFE_ALLOW_NEWLINES" || return + done + + pass_test +} + +# Test: set_unsafe_allow_newlines normalises falsy inputs +test_set_unsafe_allow_newlines_falsy_variants() { + start_test "set_unsafe_allow_newlines accepts falsy variants" + + init_logger --quiet + + for val in no off 0 NO Off FALSE; do + set_unsafe_allow_newlines "$val" + assert_equals "false" "$LOG_UNSAFE_ALLOW_NEWLINES" || return + done + + pass_test +} + +# Test: set_unsafe_allow_newlines rejects invalid input +test_set_unsafe_allow_newlines_invalid_input() { + start_test "set_unsafe_allow_newlines rejects invalid value and preserves state" + + init_logger --quiet + LOG_UNSAFE_ALLOW_NEWLINES="false" + + local stderr_file="$TEST_DIR/stderr.txt" + set_unsafe_allow_newlines "maybe" 2>"$stderr_file" + assert_not_equals 0 "$?" || return + assert_equals "false" "$LOG_UNSAFE_ALLOW_NEWLINES" || return + assert_file_contains "$stderr_file" "invalid value" || return + + pass_test +} + # Test: set_unsafe_allow_ansi_codes function test_set_unsafe_allow_ansi_codes() { start_test "set_unsafe_allow_ansi_codes changes setting" @@ -592,6 +770,50 @@ test_set_unsafe_allow_ansi_codes() { pass_test } +# Test: set_unsafe_allow_ansi_codes normalises truthy inputs +test_set_unsafe_allow_ansi_codes_truthy_variants() { + start_test "set_unsafe_allow_ansi_codes accepts truthy variants" + + init_logger --quiet + + for val in yes on 1 YES On TRUE; do + set_unsafe_allow_ansi_codes "$val" + assert_equals "true" "$LOG_UNSAFE_ALLOW_ANSI_CODES" || return + done + + pass_test +} + +# Test: set_unsafe_allow_ansi_codes normalises falsy inputs +test_set_unsafe_allow_ansi_codes_falsy_variants() { + start_test "set_unsafe_allow_ansi_codes accepts falsy variants" + + init_logger --quiet + + for val in no off 0 NO Off FALSE; do + set_unsafe_allow_ansi_codes "$val" + assert_equals "false" "$LOG_UNSAFE_ALLOW_ANSI_CODES" || return + done + + pass_test +} + +# Test: set_unsafe_allow_ansi_codes rejects invalid input +test_set_unsafe_allow_ansi_codes_invalid_input() { + start_test "set_unsafe_allow_ansi_codes rejects invalid value and preserves state" + + init_logger --quiet + LOG_UNSAFE_ALLOW_ANSI_CODES="false" + + local stderr_file="$TEST_DIR/stderr.txt" + set_unsafe_allow_ansi_codes "enabled" 2>"$stderr_file" + assert_not_equals 0 "$?" || return + assert_equals "false" "$LOG_UNSAFE_ALLOW_ANSI_CODES" || return + assert_file_contains "$stderr_file" "invalid value" || return + + pass_test +} + # Run all tests test_set_log_level test_set_log_level_string @@ -602,10 +824,18 @@ test_set_timezone_utc test_set_timezone_local test_set_journal_logging test_set_journal_tag +test_set_journal_tag_rejects_empty +test_set_journal_tag_rejects_long_tag +test_set_journal_tag_rejects_metacharacters +test_set_journal_tag_rejects_control_chars +test_set_journal_tag_preserves_tag_on_rejection test_set_syslog_facility_valid test_set_syslog_facility_invalid test_set_syslog_facility_reflects_change test_set_color_mode +test_set_color_mode_rejects_invalid +test_set_color_mode_preserves_state_on_invalid +test_set_color_mode_rejects_wrong_case test_multiple_runtime_changes test_runtime_changes_dont_affect_history test_set_log_level_verbose_interaction @@ -621,4 +851,10 @@ test_set_journal_logging_no_logger test_set_journal_tag_runtime test_journal_logging_disables_after_logger_failure test_set_unsafe_allow_newlines +test_set_unsafe_allow_newlines_truthy_variants +test_set_unsafe_allow_newlines_falsy_variants +test_set_unsafe_allow_newlines_invalid_input test_set_unsafe_allow_ansi_codes +test_set_unsafe_allow_ansi_codes_truthy_variants +test_set_unsafe_allow_ansi_codes_falsy_variants +test_set_unsafe_allow_ansi_codes_invalid_input