Skip to content

Commit 2f64f6e

Browse files
claytondukesclaytondukes
committed
fix(review): trailing-slash port strip, anchored loopback match, surfaced curl errors
Fable adversarial review findings on 8e26e2d, all six fixed: - MED f1: host_part sed reordered (scheme, trailing slashes, THEN :80) - 'http://localhost:80/' previously kept the port and retried TLS against the plaintext port, so the exact HTTPS host this fix targets still died. RED: pre-fix code fails the two new trailing-slash assertions. - LOW f2: loopback classification is now an anchored regex (localhost|127.0.0.1|[::1] with optional numeric port), not a glob - 'localhost:80@evil.com' no longer earns the -k cert-skip. RED-covered. - LOW f3: _auth_probe captures curl stderr into _PROBE_ERR and every transport-failure warn includes it (cert mismatch vs refused vs DNS are no longer the same message). RED-covered. - LOW f4: test harness bookkeeping - grep assertions now count pass AND fail via assert/assert_not; check 5 gained a positive 'retry occurred' guard so the no-dash-k assertion can't pass vacuously. - LOW f5: dropped the wrong internal story reference from the test header. - LOW f6: README troubleshooting row documents the HTTPS/cert-hostname cause alongside the wrong-port cause. Harness: 15/15 green at HEAD; 11/15 against pre-fix 8e26e2d (the four new behavioral assertions fail, as intended).
1 parent 8e26e2d commit 2f64f6e

3 files changed

Lines changed: 64 additions & 25 deletions

File tree

‎README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ duplicate tokens. To pin a specific build, change both image tags in
5151
| Port already in use | Pick another: `sudo UI3_PORT=9090 ./install.sh`. |
5252
| `that token is ingest-only / invalid` | Use a user-level token (press Enter to have the installer create one). |
5353
| SEC / orchestration errors in the UI | `SEC_API_URL` must be `http://front:80`; the installer sets this - don't change it to `gunicorn:80`. |
54-
| `could not validate the token` | LogZilla's web port isn't on `localhost:80` here. Run `sudo LZ_HOST_URL=http://your-lz:PORT ./install.sh`. |
54+
| `could not validate the token` | Two common causes. LogZilla's web port isn't on `localhost:80` here: run `sudo LZ_HOST_URL=http://your-lz:PORT ./install.sh`. Or LogZilla is HTTPS-configured and its certificate doesn't match the probed hostname: run `sudo LZ_HOST_URL=https://your-lz-cert-hostname ./install.sh` (the warn line includes curl's exact error). |
5555

5656
## Security
5757

‎install.sh‎

Lines changed: 27 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -105,11 +105,20 @@ info "Resolving LogZilla API token"
105105

106106
# _auth_probe <token> <base_url> [curl-extra-arg...]
107107
# Curls <base_url>/api/auth; sets _PROBE_HTTP/_PROBE_BODY. Non-zero only on
108-
# transport failure (DNS/refused/timeout) - HTTP status is always captured.
108+
# transport failure (DNS/refused/timeout/TLS), with curl's own error text
109+
# captured in _PROBE_ERR so callers can surface the real reason - HTTP
110+
# status is always captured.
109111
_auth_probe() {
110-
local token="$1" url="$2" resp; shift 2
111-
resp="$(curl -sS -m 20 -w $'\n%{http_code}' "$@" \
112-
-H "Authorization: token ${token}" "${url}/api/auth" 2>/dev/null)" || return 1
112+
local token="$1" url="$2" resp errf; shift 2
113+
_PROBE_ERR=""
114+
errf="$(mktemp)"
115+
if ! resp="$(curl -sS -m 20 -w $'\n%{http_code}' "$@" \
116+
-H "Authorization: token ${token}" "${url}/api/auth" 2>"$errf")"; then
117+
_PROBE_ERR="$(cat "$errf")"
118+
rm -f "$errf"
119+
return 1
120+
fi
121+
rm -f "$errf"
113122
_PROBE_HTTP="$(printf '%s' "$resp" | tail -n1)"
114123
_PROBE_BODY="$(printf '%s' "$resp" | sed '$d')"
115124
}
@@ -127,29 +136,33 @@ _auth_probe() {
127136
validate_token() {
128137
local token="$1" base="${LZ_HOST_URL}" https_base host_part
129138
if ! _auth_probe "$token" "$base"; then
130-
warn "could not reach the LZ API at ${base}/api/auth"
139+
warn "could not reach the LZ API at ${base}/api/auth${_PROBE_ERR:+ - ${_PROBE_ERR}}"
131140
return 2
132141
fi
133142
case "$_PROBE_HTTP" in
134143
301|302|307|308)
135-
host_part="$(printf '%s' "$base" | sed -E 's#^[a-zA-Z]+://##; s#:80$##; s#/+$##')"
144+
# Strip trailing slashes BEFORE the :80 strip, or a trailing slash
145+
# shields the port ('localhost:80/' must become 'localhost', not
146+
# 'localhost:80' - retrying https against the plaintext port always
147+
# fails the TLS handshake).
148+
host_part="$(printf '%s' "$base" | sed -E 's#^[a-zA-Z]+://##; s#/+$##; s#:80$##')"
136149
https_base="https://${host_part}"
137-
case "$host_part" in
138-
localhost|localhost:*|127.0.0.1|127.0.0.1:*|\[::1\]|\[::1\]:*)
150+
# Anchored match, not a glob: 'localhost:80@evil.com' must NOT be
151+
# granted the loopback cert-skip (curl would read 'localhost:80' as
152+
# userinfo and probe evil.com with verification off).
153+
if printf '%s' "$host_part" | grep -Eq '^(localhost|127\.0\.0\.1|\[::1\])(:[0-9]+)?$'; then
139154
warn "the LZ API at ${base} redirects HTTP to HTTPS (HTTP ${_PROBE_HTTP}); retrying token validation against ${https_base} - certificate verification is skipped for this loopback-only probe"
140155
if ! _auth_probe "$token" "$https_base" -k; then
141-
warn "could not reach the LZ API at ${https_base}/api/auth after the HTTPS retry"
156+
warn "could not reach the LZ API at ${https_base}/api/auth after the HTTPS retry${_PROBE_ERR:+ - ${_PROBE_ERR}}"
142157
return 2
143158
fi
144-
;;
145-
*)
159+
else
146160
warn "the LZ API at ${base} redirects HTTP to HTTPS (HTTP ${_PROBE_HTTP}); retrying token validation against ${https_base}"
147161
if ! _auth_probe "$token" "$https_base"; then
148-
warn "could not reach the LZ API at ${https_base}/api/auth after the HTTPS retry (if its certificate does not match '${host_part}', set LZ_HOST_URL to the certificate's hostname and re-run)"
162+
warn "could not reach the LZ API at ${https_base}/api/auth after the HTTPS retry${_PROBE_ERR:+ - ${_PROBE_ERR}} (if its certificate does not match '${host_part}', set LZ_HOST_URL to the certificate's hostname and re-run)"
149163
return 2
150164
fi
151-
;;
152-
esac
165+
fi
153166
;;
154167
esac
155168
[ "$_PROBE_HTTP" = "200" ] || { warn "LZ API returned HTTP ${_PROBE_HTTP} while validating the token"; return 2; }

‎tests/validate-token.test.sh‎

Lines changed: 36 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
#!/usr/bin/env bash
22
#
3-
# Tests for install.sh's validate_token HTTP->HTTPS redirect handling
4-
# (Story 34-23-adjacent QA bug #12). Uses a mock `curl` on PATH - no
5-
# network, no LZ install needed. Run: bash tests/validate-token.test.sh
3+
# Tests for install.sh's validate_token HTTP->HTTPS redirect handling.
4+
# Uses a mock `curl` on PATH - no network, no LZ install needed.
5+
# Run: bash tests/validate-token.test.sh
66
set -u
77

88
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
@@ -33,7 +33,7 @@ case "${MOCK_MODE}" in
3333
redirect-then-unreachable)
3434
case "$url" in
3535
http://*) printf '\n301' ;;
36-
https://*) exit 7 ;;
36+
https://*) echo "curl: (7) Failed to connect to localhost port 443" >&2; exit 7 ;;
3737
esac ;;
3838
esac
3939
MOCK
@@ -51,7 +51,17 @@ check() { # check <desc> <expected_rc> <actual_rc>
5151
if [ "$2" = "$3" ]; then pass=$((pass+1)); echo "ok - $1";
5252
else fail=$((fail+1)); echo "FAIL - $1 (expected rc=$2, got rc=$3)"; fi
5353
}
54-
has_log() { grep -q "$2" "$1"; }
54+
assert() { # assert <desc> <command...> - counts pass AND fail
55+
local desc="$1"; shift
56+
if "$@"; then pass=$((pass+1)); echo "ok - $desc";
57+
else fail=$((fail+1)); echo "FAIL - $desc"; fi
58+
}
59+
assert_not() { # assert_not <desc> <command...>
60+
local desc="$1"; shift
61+
if "$@"; then fail=$((fail+1)); echo "FAIL - $desc";
62+
else pass=$((pass+1)); echo "ok - $desc"; fi
63+
}
64+
retry_used_k() { grep -- "-k" "$MOCK_LOG" | grep -q "$1"; }
5565

5666
# 1. Plain HTTP 200 + user object -> 0
5767
export MOCK_LOG="${WORK}/log1"; : > "$MOCK_LOG"
@@ -62,24 +72,40 @@ validate_token tok; check "http 200 user-scoped -> 0" 0 $?
6272
export MOCK_LOG="${WORK}/log2"; : > "$MOCK_LOG"
6373
export MOCK_MODE=redirect-then-ok LZ_HOST_URL="http://localhost:80"
6474
validate_token tok; check "301 -> https retry user-scoped -> 0" 0 $?
65-
has_log "$MOCK_LOG" "https://localhost/api/auth" && echo "ok - retried against https://localhost" || { echo "FAIL - no https retry logged"; fail=$((fail+1)); }
66-
grep -- "-k" "$MOCK_LOG" | grep -q "https://localhost" && echo "ok - loopback retry skipped cert verification (-k)" || { echo "FAIL - loopback retry missing -k"; fail=$((fail+1)); }
75+
assert "retried against https://localhost" grep -q "https://localhost/api/auth" "$MOCK_LOG"
76+
assert "loopback retry skipped cert verification (-k)" retry_used_k "https://localhost/api/auth"
6777

6878
# 3. HTTP 301 -> https retry says ingest-only -> 1
6979
export MOCK_LOG="${WORK}/log3"; : > "$MOCK_LOG"
7080
export MOCK_MODE=redirect-then-null LZ_HOST_URL="http://localhost:80"
7181
validate_token tok; check "301 -> https retry ingest-only -> 1" 1 $?
7282

73-
# 4. HTTP 301 -> https unreachable -> 2
83+
# 4. HTTP 301 -> https unreachable -> 2, and curl's stderr reaches the warn
7484
export MOCK_LOG="${WORK}/log4"; : > "$MOCK_LOG"
7585
export MOCK_MODE=redirect-then-unreachable LZ_HOST_URL="http://localhost:80"
7686
validate_token tok; check "301 -> https unreachable -> 2" 2 $?
87+
assert "unreachable warn includes curl's own error" grep -q "WARN: .*Failed to connect" "$MOCK_LOG"
7788

78-
# 5. Non-loopback host: https retry WITHOUT -k
89+
# 5. Non-loopback host: https retry happens, WITHOUT -k
7990
export MOCK_LOG="${WORK}/log5"; : > "$MOCK_LOG"
8091
export MOCK_MODE=redirect-then-ok LZ_HOST_URL="http://lz.example.com"
8192
validate_token tok; check "non-loopback 301 -> https retry -> 0" 0 $?
82-
grep "https://lz.example.com" "$MOCK_LOG" | grep -q -- "-k" && { echo "FAIL - non-loopback retry must NOT use -k"; fail=$((fail+1)); } || echo "ok - non-loopback retry kept cert verification"
93+
assert "non-loopback https retry occurred" grep -q "https://lz.example.com/api/auth" "$MOCK_LOG"
94+
assert_not "non-loopback retry must NOT use -k" retry_used_k "https://lz.example.com"
95+
96+
# 6. Trailing slash: :80 must still be stripped ('localhost:80/' would
97+
# otherwise retry https against the plaintext port and always fail TLS)
98+
export MOCK_LOG="${WORK}/log6"; : > "$MOCK_LOG"
99+
export MOCK_MODE=redirect-then-ok LZ_HOST_URL="http://localhost:80/"
100+
validate_token tok; check "trailing slash 301 -> https retry -> 0" 0 $?
101+
assert "trailing-slash retry targets https://localhost (port stripped)" grep -q "https://localhost/api/auth" "$MOCK_LOG"
102+
assert_not "trailing-slash retry did not target https://localhost:80" grep -q "https://localhost:80/api/auth" "$MOCK_LOG"
103+
104+
# 7. Userinfo trick: 'localhost:80@evil.com' is NOT loopback -> no -k
105+
export MOCK_LOG="${WORK}/log7"; : > "$MOCK_LOG"
106+
export MOCK_MODE=redirect-then-ok LZ_HOST_URL="http://localhost:80@evil.com/"
107+
validate_token tok; check "userinfo-host 301 -> verified https retry -> 0" 0 $?
108+
assert_not "userinfo host must NOT get the loopback cert-skip" retry_used_k "@evil.com"
83109

84110
echo
85111
echo "passed=$pass failed=$fail"

0 commit comments

Comments
 (0)