Skip to content

fix(arcup): guard curl 8.14.x retry/exit-code bug in download paths #318

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
batuhankocyigit wants to merge 5 commits into
base: main
Choose a base branch
from
+216 −5
Open

fix(arcup): guard curl 8.14.x retry/exit-code bug in download paths #318

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cb26369
fix(arcup): guard install script's own curl call against same exit-co…
batuhankocyigit Sep 2, 2026
384833d
Update install
batuhankocyigit Sep 2, 2026
59f2557
Merge branch 'circlefin:main' into fix/curl-retry-exit-code-8-14
batuhankocyigit Sep 3, 2026
bdcfc7d
test(arcup): add regression tests for curl 8.14.x silent-success bug
batuhankocyigit Sep 3, 2026
682d6a0
Bump installer version to 0.2.1
batuhankocyigit Sep 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions arcup/arcup
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ set -euo pipefail

# NOTE: if you make modifications to this script, please increment the version number.
# WARNING: the SemVer pattern: major.minor.patch must be followed as we use it to determine if the script is up to date.
ARCUP_INSTALLER_VERSION="0.2.0"
ARCUP_INSTALLER_VERSION="0.2.1"

REPO="${ARC_REPO:-circlefin/arc-node}"
if [[ -n "${ARC_REPO:-}" ]] && [[ ! "$ARC_REPO" =~ ^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$ ]]; then
Expand Down Expand Up @@ -324,7 +324,7 @@ download_file() {
return
fi

if ! curl_with_headers -#fL --max-time 900 -o "$output_path" "$url"; then
if ! curl_with_headers -#fL --max-time 900 -o "$output_path" "$url" || [[ ! -s "$output_path" ]]; then

@osr21 osr21 Sep 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified this path is genuinely in scope: curl_with_headers (arcup:79–84) prepends CURL_RETRY_ARGS=(--retry 3 ...) on both branches, so the 8.14.x exit-code bug can fire here.

Worth noting for reviewers weighing severity: an empty archive reaching this point would still be caught downstream by verify_checksum_file and tar -tzf, so this one fails safe today. The guard's real value is converting a confusing downstream checksum error into an accurate failure at the point it actually happened. Correct change, lower severity than install:75.

rm -f "$output_path"
download_error "$tag" "$filename" "$url"
fi
Expand All @@ -350,7 +350,7 @@ download_file_with_github_api() {
download_url=$(resolve_github_asset_download_url "$asset_api_url") || return 1

if curl "${CURL_RETRY_ARGS[@]}" -fL --max-time 900 -o "$output_path" "$download_url" >/dev/null 2>&1; then
[[ -f "$output_path" ]] && return 0
[[ -s "$output_path" ]] && return 0
fi

rm -f "$output_path"
Expand All @@ -374,7 +374,7 @@ download_file_with_gh() {
--dir "$output_dir" \
--clobber \
>/dev/null 2>&1; then
[[ -f "$output_path" ]] && return 0
[[ -s "$output_path" ]] && return 0

@osr21 osr21 Sep 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a fourth production change that isn't in the PR summary's three bullets, and it's the only changed call site with no regression test.

It's also not the curl 8.14.x bug — this branch shells out to gh release download, not curl, so --retry never enters the picture. It reads as unrelated (and welcome) hardening against gh leaving a zero-byte artifact behind.

No correctness objection; -s can't false-reject here since no real release asset is zero bytes. But the summary and commit message should describe it as separate hardening, otherwise the history implies curl was at fault on a path curl never touches.

fi

rm -f "$output_path"
Expand Down
2 changes: 1 addition & 1 deletion arcup/install
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ main() {
error "curl not found. Please install curl."
fi

if ! curl_with_headers -#fL --max-time 300 -o "$_INSTALL_TMP_FILE" "$ARCUP_URL" ; then
if ! curl_with_headers -#fL --max-time 300 -o "$_INSTALL_TMP_FILE" "$ARCUP_URL" || [[ ! -s "$_INSTALL_TMP_FILE" ]]; then

@osr21 osr21 Sep 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the change that carries the actual severity, and the PR body is right to single it out: there is no checksum, signature or archive-integrity step anywhere in install, so pre-fix a 0-byte download went straight through install_bin and got chmod +x'd into place. Nothing downstream would have caught it.

Heads-up on overlap: this hunk is byte-identical to the one in #314, which is also open and also closes #309. I tested it — #314 merges clean onto main and this PR merges clean on top, so nothing will break either way, but merging both is redundant. #318 is a superset of #314's fix except for the ARCUP_INSTALLER_VERSION bump, which only #314 has. Worth someone deciding which lands rather than leaving both blocked.

error "failed to download arcup from '$ARCUP_URL'"
fi

Expand Down
211 changes: 211 additions & 0 deletions arcup/test_arcup.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -641,6 +641,162 @@ EOF
pass "download_file falls back to curl"
}

# --- Regression tests for the curl 8.14.0/8.14.1 exit-code bug (issue #309) ---
# On these versions, curl with --retry set but not triggered (permanent
# failures like 404) can exit 0 while leaving an empty/absent output file.
# These tests simulate exactly that shape: exit 0, empty file.

test_download_file_rejects_empty_success_from_curl_fallback() {
local fakebin="$TEST_TMP/empty-success-fakebin"
local output_dir="$TEST_TMP/empty-success-output"
local asset_name="arc-node-v1.2.3-aarch64-apple-darwin.tar.gz"

mkdir -p "$fakebin" "$output_dir"

# gh not available, so download_file falls through to the bare curl call.
cat > "$fakebin/gh" <<'EOF'
#!/usr/bin/env bash
exit 1
EOF
chmod 755 "$fakebin/gh"

cat > "$fakebin/curl" <<'EOF'
#!/usr/bin/env bash
set -euo pipefail

out=""
url=""
while [[ $# -gt 0 ]]; do
case "$1" in
-o)
out="$2"
shift 2
;;
--retry | --retry-delay | --connect-timeout | --max-time)
shift 2
;;
-*)
shift
;;
*)
url="$1"
shift
;;
esac
done

: > "$out"
exit 0
EOF
chmod 755 "$fakebin/curl"

if (
PATH="$fakebin:$PATH"
GITHUB_AUTH_TOKEN=""
CURL_HEADERS=()
REPO="test/arc-node"
download_file "v1.2.3" "$asset_name" "$output_dir"
) >"$TEST_TMP/empty-success.out" 2>&1; then
cat "$TEST_TMP/empty-success.out" >&2
fail "download_file rejects curl's empty-file success (8.14.x bug)"
fi

if [[ -e "$output_dir/$asset_name" ]]; then
fail "download_file must not leave an empty file behind"
fi

pass "download_file rejects curl's empty-file success (8.14.x bug)"
}

test_download_file_with_github_api_rejects_empty_success() {
local fakebin="$TEST_TMP/empty-token-api-fakebin"
local output_dir="$TEST_TMP/empty-token-api-output"
local asset_name="arc-node-v1.2.3-aarch64-apple-darwin.tar.gz"

mkdir -p "$fakebin" "$output_dir"

cat > "$fakebin/curl" <<'EOF'
#!/usr/bin/env bash
set -euo pipefail

out=""
dump=""
url=""
while [[ $# -gt 0 ]]; do
case "$1" in
-o)
out="$2"
shift 2
;;
-D)
dump="$2"
shift 2
;;
-H | --retry | --retry-delay | --connect-timeout | --max-time)
shift 2
;;
-*)
shift
;;
*)
url="$1"
shift
;;
esac
done

case "$url" in
*api.github.com/repos/test/arc-node/releases/tags/v1.2.3)
data='{"assets":[{"url":"https://api.github.com/repos/test/arc-node/releases/assets/123","name":"arc-node-v1.2.3-aarch64-apple-darwin.tar.gz"}]}'
;;
*api.github.com/repos/test/arc-node/releases/assets/123)
if [[ "$dump" == "-" ]]; then
printf 'HTTP/1.1 302 Found\r\nLocation: https://objects.example/arc-node-v1.2.3-aarch64-apple-darwin.tar.gz\r\n\r\n'
exit 0
fi
exit 22
;;
*objects.example/arc-node-v1.2.3-aarch64-apple-darwin.tar.gz)
# curl 8.14.x bug: exits 0 on the final asset download even
# though the permanent-failure response body is empty.
: > "$out"
exit 0
;;
*)
printf 'unexpected curl URL: %s\n' "$url" >&2
exit 22
;;
esac

if [[ -n "$out" ]]; then
printf '%s\n' "$data" > "$out"
else
printf '%s\n' "$data"
fi
EOF
chmod 755 "$fakebin/curl"

(
PATH="$fakebin:$PATH"
GITHUB_AUTH_TOKEN="test-token"
CURL_HEADERS=()
REPO="test/arc-node"
if download_file_with_github_api "v1.2.3" "$asset_name" "$output_dir"; then
exit 1
fi
exit 0
) >"$TEST_TMP/empty-token-api.out" 2>&1 || {
cat "$TEST_TMP/empty-token-api.out" >&2
fail "download_file_with_github_api rejects curl's empty-file success (8.14.x bug)"
}

if [[ -e "$output_dir/$asset_name" ]]; then
fail "download_file_with_github_api must not leave an empty file behind"
fi

pass "download_file_with_github_api rejects curl's empty-file success (8.14.x bug)"
}

test_fixture_install_matrix() {
local fixture_dir="$TEST_TMP/fixture"
local fakebin="$TEST_TMP/fakebin"
Expand Down Expand Up @@ -810,6 +966,58 @@ test_install_binary_rejects_symlink() {
pass "install_binary rejects symlink"
}

# --- Regression test for arcup/install's own curl call (issue #309) ---

test_install_rejects_empty_arcup_download() {
local fakebin="$TEST_TMP/install-empty-fakebin"
local arc_dir="$TEST_TMP/install-empty-arc-dir"

mkdir -p "$fakebin"

cat > "$fakebin/curl" <<'EOF'
#!/usr/bin/env bash
set -euo pipefail

out=""
while [[ $# -gt 0 ]]; do
case "$1" in
-o)
out="$2"
shift 2
;;
--retry | --retry-delay | --connect-timeout | --max-time)
shift 2
;;
-*)
shift
;;
*)
shift
;;
esac
done

: > "$out"
exit 0
EOF
chmod 755 "$fakebin/curl"

if (
PATH="$fakebin:$PATH"
ARC_DIR="$arc_dir"
bash "$ROOT_DIR/arcup/install"
) >"$TEST_TMP/install-empty.out" 2>&1; then
cat "$TEST_TMP/install-empty.out" >&2
fail "install rejects empty arcup download (8.14.x bug)"
fi

if [[ -x "$arc_dir/bin/arcup" ]]; then
fail "install must not leave an empty executable behind"
fi

pass "install rejects empty arcup download (8.14.x bug)"
}

test_version_normalization
test_version_comparison
test_target_mapping
Expand All @@ -823,7 +1031,10 @@ test_latest_version_retries_anonymous_after_token_failure
test_latest_version_redacts_authenticated_failure
test_download_file_uses_gh_when_available
test_download_file_falls_back_to_curl
test_download_file_rejects_empty_success_from_curl_fallback
test_download_file_with_github_api_rejects_empty_success
test_fixture_install_matrix
test_archive_path_traversal_fails
test_archive_link_entries_fail
test_install_binary_rejects_symlink
test_install_rejects_empty_arcup_download