address concerns of #1952 by aalves08 · Pull Request #70 · rancher/partner-extensions

aalves08 · 2026-03-31T10:31:57Z

Address concerns of reporting issue

aalves08 · 2026-03-31T10:32:09Z

@macedogm could you also take a look to make that your teams standpoint changes look good?

@richard-cox as the master of all UI code, please have a look as well 🙏

richard-cox · 2026-03-31T15:15:54Z

Contributes to https://github.com/rancher/rancher-security/issues/1952

macedogm

LGTM, although I don't understand all the logic of this repo. So please seek @richard-cox's approval too.

From what I saw, the icons will come from:

      ICON=$(yq eval '.icon' ${CHART_FILE})

Right?

aalves08 · 2026-04-01T12:57:38Z

@macedogm what this script does is pull the helm chart artifacts for it's upstream repository, patches them (some references need to change) and put's them in this repo.

One of the parts is the icon bit, that needs to be fetched and it's name patched so that it appears fine on Rancher Dashboard when a person is viewing the card related to the extension.

Since this is partners related, we have to trust the source, but the improvements I did in terms of url validation seem to limit the concerns as best as we can.

@richard-cox are you happy with the solution?

macedogm · 2026-04-01T18:06:35Z

@aalves08 thanks for the explanation. Would be good to get Copilot for a syntax review too, please.

Copilot

Pull request overview

This PR hardens the scripts/sync asset sync workflow by adding validation around extension icon downloads to mitigate SSRF risk during chart processing.

Changes:

Added a validate_icon_url helper to enforce HTTPS and restrict icon hosts to GitHub domains.
Switched icon download logic from wget to curl and integrated URL validation before attempting the download.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-02T08:59:58Z

scripts/sync

-        ERR=$?
-        set -1
+        if validate_icon_url "${ICON}"; then
+          curl --proto '=https' --proto-redir '=https' --location --silent --output "${BASE_DIR}/${ICON_REL}" "${ICON}"


curl is invoked without --fail/--fail-with-body, so HTTP 4xx/5xx responses will still return exit code 0 and be treated as a successful icon download. This can result in saving an error page as the icon and patching Chart.yaml/package.json to reference it. Consider adding --fail-with-body (or --fail) and optionally --show-error so ERR correctly reflects HTTP errors.

Suggested change

curl --proto '=https' --proto-redir '=https' --location --silent --output "${BASE_DIR}/${ICON_REL}" "${ICON}"

curl --proto '=https' --proto-redir '=https' --location --silent --show-error --fail-with-body --output "${BASE_DIR}/${ICON_REL}" "${ICON}"

Copilot · 2026-04-02T08:59:58Z

scripts/sync

-        ERR=$?
-        set -1
+        if validate_icon_url "${ICON}"; then
+          curl --proto '=https' --proto-redir '=https' --location --silent --output "${BASE_DIR}/${ICON_REL}" "${ICON}"


URL validation is performed only on the initial ICON value, but curl --location will follow redirects to any HTTPS host. That means a URL on an allowlisted host that responds with a redirect can bypass the host allowlist and fetch from a different host. To keep the SSRF protection effective, consider either (a) disallowing redirects (remove --location) and require direct raw.githubusercontent.com URLs, or (b) resolve/validate each redirect target before following it.

Suggested change

curl --proto '=https' --proto-redir '=https' --location --silent --output "${BASE_DIR}/${ICON_REL}" "${ICON}"

curl --proto '=https' --proto-redir '=https' --silent --output "${BASE_DIR}/${ICON_REL}" "${ICON}"

Copilot · 2026-04-02T08:59:58Z

scripts/sync

+        # Validate and download the icon - ignore errors, we will handle those
        set +e
-        wget --quiet -O ${BASE_DIR}/${ICON_REL} ${ICON}
-        ERR=$?
-        set -1
+        if validate_icon_url "${ICON}"; then
+          curl --proto '=https' --proto-redir '=https' --location --silent --output "${BASE_DIR}/${ICON_REL}" "${ICON}"
+          ERR=$?
+        else
+          echo -e "${YELLOW}${BOLD}Warning: Icon URL failed validation - skipping download${RESET}"
+          ERR=1
+        fi
+        set -e


Instead of toggling set +e/set -e around the download, consider restructuring the logic so failures are handled in an if condition (e.g., combine the curl call into the conditional). This avoids temporarily disabling errexit and makes it less likely future edits accidentally run with -e off.

Copilot · 2026-04-02T08:59:59Z

scripts/sync


+# Validation function for icon URLs to prevent SSRF attacks
+validate_icon_url() {
+  local url=$1


In validate_icon_url, local url=$1 leaves $1 subject to word-splitting/globbing if it ever contains unexpected characters. Prefer local url="$1" for safer handling and consistency with the rest of the function’s quoting.

Suggested change

local url=$1

local url="$1"

- align scripts/sync validate_icon_url more with rancher/ui-plugin-charts - use curl with location... but ensure if redirected it passes original url requirements

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-02T15:36:53Z

scripts/sync

+        if validate_icon_url "${ICON}"; then
+          EFFECTIVE_URL=$(curl --proto '=https' --proto-redir '=https' --fail --location --silent --output "${BASE_DIR}/${ICON_REL}" --write-out '%{url_effective}' "${ICON}")
+          ERR=$?
+          if [ $ERR -eq 0 ] && ! validate_icon_url "${EFFECTIVE_URL}"; then
+            echo -e "${YELLOW}${BOLD}Warning: Redirected icon URL failed validation - discarding${RESET}"
+            rm -f "${BASE_DIR}/${ICON_REL}"
+            ERR=1
+          fi
+        else
+          echo -e "${YELLOW}${BOLD}Warning: Icon URL failed validation - skipping download${RESET}"
+          ERR=1
+        fi


validate_icon_url checks the final redirected URL after curl --location has already followed redirects and performed the outbound requests. If the goal is SSRF prevention, this still allows redirects to untrusted hosts (the request happens before the post-download validation).

Consider validating the redirect chain before issuing a request to the next hop (e.g., disable automatic redirects with --max-redirs 0 and manually follow Location while validating each intermediate URL, or fetch headers first, validate, then download without further redirects).

Suggested change

if validate_icon_url "${ICON}"; then

EFFECTIVE_URL=$(curl --proto '=https' --proto-redir '=https' --fail --location --silent --output "${BASE_DIR}/${ICON_REL}" --write-out '%{url_effective}' "${ICON}")

ERR=$?

if [ $ERR -eq 0 ] && ! validate_icon_url "${EFFECTIVE_URL}"; then

echo -e "${YELLOW}${BOLD}Warning: Redirected icon URL failed validation - discarding${RESET}"

rm -f "${BASE_DIR}/${ICON_REL}"

ERR=1

fi

else

echo -e "${YELLOW}${BOLD}Warning: Icon URL failed validation - skipping download${RESET}"

ERR=1

fi

# Manually resolve redirects, validating each hop before following it.

RESOLVED_URL="${ICON}"

MAX_ICON_REDIRECTS=5

ICON_REDIRECT_COUNT=0

ERR=0

while [ ${ICON_REDIRECT_COUNT} -lt ${MAX_ICON_REDIRECTS} ]; do

# Validate the current URL before issuing a request

if ! validate_icon_url "${RESOLVED_URL}"; then

echo -e "${YELLOW}${BOLD}Warning: Icon URL failed validation - skipping download${RESET}"

ERR=1

break

fi

# Fetch headers only, do not automatically follow redirects

ICON_CURL_OUTPUT=$(curl --proto '=https' --proto-redir '=https' --fail --silent --show-error --head --max-redirs 0 --write-out '%{http_code} %{redirect_url}' --output /dev/null "${RESOLVED_URL}")

CURL_STATUS=$?

if [ ${CURL_STATUS} -ne 0 ]; then

ERR=${CURL_STATUS}

break

fi

ICON_HTTP_CODE=$(echo "${ICON_CURL_OUTPUT}" | awk '{print $1}')

ICON_REDIRECT_URL=$(echo "${ICON_CURL_OUTPUT}" | cut -d' ' -f2-)

# If 3xx with a redirect URL, follow it (after validation in next loop iteration)

if [ "${ICON_HTTP_CODE}" -ge 300 ] && [ "${ICON_HTTP_CODE}" -lt 400 ] && [ -n "${ICON_REDIRECT_URL}" ] && [ "${ICON_REDIRECT_URL}" != "null" ]; then

RESOLVED_URL="${ICON_REDIRECT_URL}"

ICON_REDIRECT_COUNT=$((ICON_REDIRECT_COUNT + 1))

continue

fi

# Non-redirect status; RESOLVED_URL is the final URL

break

done

if [ ${ERR} -eq 0 ] && [ ${ICON_REDIRECT_COUNT} -ge ${MAX_ICON_REDIRECTS} ]; then

echo -e "${YELLOW}${BOLD}Warning: Icon URL exceeded maximum redirects - skipping download${RESET}"

ERR=1

fi

if [ ${ERR} -eq 0 ]; then

# Final safety check on the resolved URL

if ! validate_icon_url "${RESOLVED_URL}"; then

echo -e "${YELLOW}${BOLD}Warning: Resolved icon URL failed validation - skipping download${RESET}"

ERR=1

else

curl --proto '=https' --proto-redir '=https' --fail --silent --show-error --output "${BASE_DIR}/${ICON_REL}" "${RESOLVED_URL}"

ERR=$?

fi

fi

richard-cox · 2026-04-02T15:40:21Z

Same scenario as rancher/ui-plugin-charts#206 (comment)

I'll follow the same advice there and seek a re-review from @aalves08

address concerns

baa1dfe

aalves08 requested review from macedogm and richard-cox March 31, 2026 10:31

aalves08 self-assigned this Mar 31, 2026

macedogm approved these changes Mar 31, 2026

View reviewed changes

richard-cox requested a review from Copilot April 2, 2026 08:57

Copilot started reviewing on behalf of richard-cox April 2, 2026 08:57 View session

Copilot AI reviewed Apr 2, 2026

View reviewed changes

Changes following copilot review

b94a7c5

- align scripts/sync validate_icon_url more with rancher/ui-plugin-charts - use curl with location... but ensure if redirected it passes original url requirements

richard-cox requested a review from Copilot April 2, 2026 15:33

Copilot started reviewing on behalf of richard-cox April 2, 2026 15:34 View session

Copilot AI reviewed Apr 2, 2026

View reviewed changes

richard-cox requested a review from macedogm April 2, 2026 15:39

	curl --proto '=https' --proto-redir '=https' --location --silent --output "${BASE_DIR}/${ICON_REL}" "${ICON}"
	curl --proto '=https' --proto-redir '=https' --location --silent --show-error --fail-with-body --output "${BASE_DIR}/${ICON_REL}" "${ICON}"

-        if validate_icon_url "${ICON}"; then
-          EFFECTIVE_URL=$(curl --proto '=https' --proto-redir '=https' --fail --location --silent --output "${BASE_DIR}/${ICON_REL}" --write-out '%{url_effective}' "${ICON}")
-          ERR=$?
-          if [ $ERR -eq 0 ] && ! validate_icon_url "${EFFECTIVE_URL}"; then
-            echo -e "${YELLOW}${BOLD}Warning: Redirected icon URL failed validation - discarding${RESET}"
-            rm -f "${BASE_DIR}/${ICON_REL}"
-            ERR=1
-          fi
-        else
-          echo -e "${YELLOW}${BOLD}Warning: Icon URL failed validation - skipping download${RESET}"
-          ERR=1
-        fi
+        # Manually resolve redirects, validating each hop before following it.
+        RESOLVED_URL="${ICON}"
+        MAX_ICON_REDIRECTS=5
+        ICON_REDIRECT_COUNT=0
+        ERR=0
+        while [ ${ICON_REDIRECT_COUNT} -lt ${MAX_ICON_REDIRECTS} ]; do
+          # Validate the current URL before issuing a request
+          if ! validate_icon_url "${RESOLVED_URL}"; then
+            echo -e "${YELLOW}${BOLD}Warning: Icon URL failed validation - skipping download${RESET}"
+            ERR=1
+            break
+          fi
+          # Fetch headers only, do not automatically follow redirects
+          ICON_CURL_OUTPUT=$(curl --proto '=https' --proto-redir '=https' --fail --silent --show-error --head --max-redirs 0 --write-out '%{http_code} %{redirect_url}' --output /dev/null "${RESOLVED_URL}")
+          CURL_STATUS=$?
+          if [ ${CURL_STATUS} -ne 0 ]; then
+            ERR=${CURL_STATUS}
+            break
+          fi
+          ICON_HTTP_CODE=$(echo "${ICON_CURL_OUTPUT}" | awk '{print $1}')
+          ICON_REDIRECT_URL=$(echo "${ICON_CURL_OUTPUT}" | cut -d' ' -f2-)
+          # If 3xx with a redirect URL, follow it (after validation in next loop iteration)
+          if [ "${ICON_HTTP_CODE}" -ge 300 ] && [ "${ICON_HTTP_CODE}" -lt 400 ] && [ -n "${ICON_REDIRECT_URL}" ] && [ "${ICON_REDIRECT_URL}" != "null" ]; then
+            RESOLVED_URL="${ICON_REDIRECT_URL}"
+            ICON_REDIRECT_COUNT=$((ICON_REDIRECT_COUNT + 1))
+            continue
+          fi
+          # Non-redirect status; RESOLVED_URL is the final URL
+          break
+        done
+        if [ ${ERR} -eq 0 ] && [ ${ICON_REDIRECT_COUNT} -ge ${MAX_ICON_REDIRECTS} ]; then
+          echo -e "${YELLOW}${BOLD}Warning: Icon URL exceeded maximum redirects - skipping download${RESET}"
+          ERR=1
+        fi
+        if [ ${ERR} -eq 0 ]; then
+          # Final safety check on the resolved URL
+          if ! validate_icon_url "${RESOLVED_URL}"; then
+            echo -e "${YELLOW}${BOLD}Warning: Resolved icon URL failed validation - skipping download${RESET}"
+            ERR=1
+          else
+            curl --proto '=https' --proto-redir '=https' --fail --silent --show-error --output "${BASE_DIR}/${ICON_REL}" "${RESOLVED_URL}"
+            ERR=$?
+          fi
+        fi

Conversation

aalves08 commented Mar 31, 2026

Uh oh!

aalves08 commented Mar 31, 2026

Uh oh!

richard-cox commented Mar 31, 2026

Uh oh!

macedogm left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aalves08 commented Apr 1, 2026

Uh oh!

macedogm commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

richard-cox commented Apr 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

macedogm left a comment •

edited

Loading

macedogm commented Apr 1, 2026 •

edited

Loading