Skip to content

Sanitize Control Plane org names in production promotion #759

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

Merged
justin808 merged 1 commit into from
Jun 2, 2026
Merged

Sanitize Control Plane org names in production promotion #759

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
69 changes: 58 additions & 11 deletions .github/workflows/cpflow-promote-staging-to-production.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,53 @@ jobs:
variable:STAGING_APP_NAME
variable:PRODUCTION_APP_NAME

- name: Normalize Control Plane org names
id: cpln-orgs
env:
CPLN_ORG_STAGING: ${{ vars.CPLN_ORG_STAGING }}
CPLN_ORG_PRODUCTION: ${{ vars.CPLN_ORG_PRODUCTION }}
shell: bash
run: |
set -euo pipefail

sanitize_control_plane_name() {
local label="$1"
local value="$2"

value="${value#"${value%%[![:space:]]*}"}"
value="${value%"${value##*[![:space:]]}"}"
Comment on lines +125 to +126

@claude claude Bot Jun 2, 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.

These lines implement a pure-bash leading/trailing whitespace trim via nested parameter expansion — a correct but fairly cryptic idiom. The logic is:

  • Line 125: ${value%%[![:space:]]*} strips everything from the first non-space char to the end, leaving just the leading whitespace prefix; then ${value#...} removes that prefix.
  • Line 126: mirror operation for the trailing side.

This is intentionally fork-free, but a future maintainer is likely to stumble on it. A short comment explaining the intent (pure-bash trim, avoids subprocesses) would help. Alternatively, value="$(printf '%s' "${value}" | sed -E 's/^[[:space:]]+|[[:space:]]+$//g')" is more readable at the cost of one sed subprocess — acceptable here since this step already runs bash.

Suggested change
value="${value#"${value%%[![:space:]]*}"}"
value="${value%"${value##*[![:space:]]}"}"
# Pure-bash leading/trailing whitespace strip (avoids a subprocess).
value="${value#"${value%%[![:space:]]*}"}"
value="${value%"${value##*[![:space:]]}"}"


if [[ "${value}" == *$'\r'* || "${value}" == *$'\n'* ]]; then
echo "::error::${label} contains embedded line endings; remove them from the repository variable instead of relying on normalization." >&2

@claude claude Bot Jun 2, 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.

Minor inconsistency: this error message is sent to stderr (>&2) while validate_control_plane_org's error (line 143) goes to stdout. GitHub Actions processes ::error:: workflow commands from both streams, so both create annotations — but stdout is the conventional channel. Suggest aligning by dropping >&2 here:

Suggested change
echo "::error::${label} contains embedded line endings; remove them from the repository variable instead of relying on normalization." >&2
echo "::error::${label} contains embedded line endings; remove them from the repository variable instead of relying on normalization."

exit 1
fi

printf '%s' "${value}"
}
Comment on lines +121 to +134

@claude claude Bot Jun 2, 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.

The function strips \r and \n but not spaces or tabs. A value like " my-org " (copy-pasted from a UI with surrounding spaces) passes through sanitized but then fails the regex with a message that says "remove whitespace" — which is technically correct but unhelpful.

Consider also stripping leading/trailing whitespace:

Suggested change
sanitize_control_plane_name() {
local value="$1"
value="${value//$'\r'/}"
value="${value//$'\n'/}"
printf '%s' "${value}"
}
sanitize_control_plane_name() {
local value="$1"
value="${value//$'\r'/}"
value="${value//$'\n'/}"
value="${value//$'\t'/}"
# strip leading/trailing spaces
value="${value#"${value%%[! ]*}"}"
value="${value%"${value##*[! ]}"}"
printf '%s' "${value}"
}

Comment on lines +121 to +134

@greptile-apps greptile-apps Bot Jun 2, 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.

P2 sanitize_control_plane_name strips only CR/LF, not spaces or tabs

The function removes \r and \n, which handles the documented copy-paste artifact. However, spaces and tabs (also common in pasted values) are passed through unchanged. The downstream validate_control_plane_org regex will then reject them with a message that says "remove whitespace, line endings, or unsupported characters" — which is correct — but the value won't actually be fixed by the sanitizer. Extending the sanitizer to also strip horizontal whitespace would make the fix symmetric and reduce operator confusion when someone copies with a trailing space instead of a newline.

Suggested change
sanitize_control_plane_name() {
local value="$1"
value="${value//$'\r'/}"
value="${value//$'\n'/}"
printf '%s' "${value}"
}
sanitize_control_plane_name() {
local value="$1"
value="${value//$'\r'/}"
value="${value//$'\n'/}"
value="${value//$' '/}"
value="${value//$'\t'/}"
printf '%s' "${value}"
}

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!


validate_control_plane_org() {
local label="$1"
local value="$2"

if ! [[ "${value}" =~ ^[a-z0-9]([a-z0-9-]*[a-z0-9])?$ ]]; then

@claude claude Bot Jun 2, 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.

The current pattern allows consecutive hyphens (e.g. my--org), which DNS labels and most cloud providers prohibit. Consider tightening to reject them:

Suggested change
if ! [[ "${value}" =~ ^[a-z0-9]([a-z0-9-]*[a-z0-9])?$ ]]; then
if ! [[ "${value}" =~ ^[a-z0-9]([a-z0-9]*(-[a-z0-9]+)*)?$ ]]; then

This rewrites the middle section as zero-or-more runs of alphanumerics optionally separated by single hyphens, so my-org passes but my--org doesn't.

local display_value
display_value="$(printf '%q' "${value}")"
echo "::error::${label} (${display_value}) must be a valid Control Plane org name; use lowercase alphanumeric characters and hyphens only, with no leading or trailing hyphen."
exit 1
fi
Comment on lines +136 to +145

@greptile-apps greptile-apps Bot Jun 2, 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.

P2 Validation error omits the offending value

The validate_control_plane_org error message names only the variable label, not the sanitized value being rejected. If validation fails for a reason other than trailing CR/LF (e.g. an upper-case letter, a leading dash, or a space that wasn't stripped), an operator looking at the Actions log won't be able to see the actual problematic value without inspecting the repository variable store manually. Including the sanitized value in the error message would halve the investigation round-trip.

}

staging_org="$(sanitize_control_plane_name "CPLN_ORG_STAGING" "${CPLN_ORG_STAGING}")"
production_org="$(sanitize_control_plane_name "CPLN_ORG_PRODUCTION" "${CPLN_ORG_PRODUCTION}")"

validate_control_plane_org "CPLN_ORG_STAGING" "${staging_org}"
validate_control_plane_org "CPLN_ORG_PRODUCTION" "${production_org}"

{
echo "staging=${staging_org}"
echo "production=${production_org}"
} >> "$GITHUB_OUTPUT"

- name: Capture release context
id: release-context
env:
Expand All @@ -127,7 +174,7 @@ jobs:
uses: ./.cpflow/.github/actions/cpflow-setup-environment
with:
token: ${{ secrets.CPLN_TOKEN_PRODUCTION }}
org: ${{ vars.CPLN_ORG_PRODUCTION }}
org: ${{ steps.cpln-orgs.outputs.production }}
working_directory: .cpflow
cpln_cli_version: ${{ vars.CPLN_CLI_VERSION }}
cpflow_version: ${{ vars.CPFLOW_VERSION }}
Expand Down Expand Up @@ -200,8 +247,8 @@ jobs:
CPLN_TOKEN_PRODUCTION: ${{ secrets.CPLN_TOKEN_PRODUCTION }}
STAGING_APP_NAME: ${{ vars.STAGING_APP_NAME }}
PRODUCTION_APP_NAME: ${{ vars.PRODUCTION_APP_NAME }}
CPLN_ORG_STAGING: ${{ vars.CPLN_ORG_STAGING }}
CPLN_ORG_PRODUCTION: ${{ vars.CPLN_ORG_PRODUCTION }}
CPLN_ORG_STAGING: ${{ steps.cpln-orgs.outputs.staging }}
CPLN_ORG_PRODUCTION: ${{ steps.cpln-orgs.outputs.production }}
shell: bash
run: |
set -euo pipefail
Expand Down Expand Up @@ -235,7 +282,7 @@ jobs:
id: capture-current
env:
PRODUCTION_APP_NAME: ${{ vars.PRODUCTION_APP_NAME }}
CPLN_ORG_PRODUCTION: ${{ vars.CPLN_ORG_PRODUCTION }}
CPLN_ORG_PRODUCTION: ${{ steps.cpln-orgs.outputs.production }}
WORKLOAD_NAMES: ${{ steps.workloads.outputs.names }}
PRIMARY_WORKLOAD: ${{ steps.workloads.outputs.primary }}
shell: bash
Expand Down Expand Up @@ -293,7 +340,7 @@ jobs:
env:
CPLN_TOKEN_STAGING: ${{ secrets.CPLN_TOKEN_STAGING }}
STAGING_APP_NAME: ${{ vars.STAGING_APP_NAME }}
CPLN_ORG_STAGING: ${{ vars.CPLN_ORG_STAGING }}
CPLN_ORG_STAGING: ${{ steps.cpln-orgs.outputs.staging }}
WORKLOAD_NAMES: ${{ steps.workloads.outputs.names }}
PRIMARY_WORKLOAD: ${{ steps.workloads.outputs.primary }}
shell: bash
Expand Down Expand Up @@ -342,8 +389,8 @@ jobs:
CPLN_TOKEN_STAGING: ${{ secrets.CPLN_TOKEN_STAGING }}
CPLN_TOKEN_PRODUCTION: ${{ secrets.CPLN_TOKEN_PRODUCTION }}
PRODUCTION_APP_NAME: ${{ vars.PRODUCTION_APP_NAME }}
CPLN_ORG_STAGING: ${{ vars.CPLN_ORG_STAGING }}
CPLN_ORG_PRODUCTION: ${{ vars.CPLN_ORG_PRODUCTION }}
CPLN_ORG_STAGING: ${{ steps.cpln-orgs.outputs.staging }}
CPLN_ORG_PRODUCTION: ${{ steps.cpln-orgs.outputs.production }}
STAGING_IMAGE: ${{ steps.staging-image.outputs.image }}
shell: bash
run: |
Expand Down Expand Up @@ -427,7 +474,7 @@ jobs:
- name: Deploy image to production
env:
PRODUCTION_APP_NAME: ${{ vars.PRODUCTION_APP_NAME }}
CPLN_ORG_PRODUCTION: ${{ vars.CPLN_ORG_PRODUCTION }}
CPLN_ORG_PRODUCTION: ${{ steps.cpln-orgs.outputs.production }}
RELEASE_PHASE_FLAG: ${{ steps.release-phase.outputs.flag }}
shell: bash
run: |
Expand All @@ -447,7 +494,7 @@ jobs:
with:
workload_name: ${{ steps.workloads.outputs.primary }}
app_name: ${{ vars.PRODUCTION_APP_NAME }}
org: ${{ vars.CPLN_ORG_PRODUCTION }}
org: ${{ steps.cpln-orgs.outputs.production }}
max_retries: ${{ env.HEALTH_CHECK_RETRIES }}
interval_seconds: ${{ env.HEALTH_CHECK_INTERVAL }}
accepted_statuses: ${{ env.HEALTH_CHECK_ACCEPTED_STATUSES }}
Expand All @@ -457,7 +504,7 @@ jobs:
env:
ROLLBACK_STATE: ${{ steps.capture-current.outputs.rollback_state }}
PRODUCTION_APP_NAME: ${{ vars.PRODUCTION_APP_NAME }}
CPLN_ORG_PRODUCTION: ${{ vars.CPLN_ORG_PRODUCTION }}
CPLN_ORG_PRODUCTION: ${{ steps.cpln-orgs.outputs.production }}
shell: bash
run: |
# Best-effort rollback: try every workload, aggregate failures, exit non-zero at the end
Expand Down Expand Up @@ -519,7 +566,7 @@ jobs:
env:
ROLLBACK_STATE: ${{ steps.capture-current.outputs.rollback_state }}
PRODUCTION_APP_NAME: ${{ vars.PRODUCTION_APP_NAME }}
CPLN_ORG_PRODUCTION: ${{ vars.CPLN_ORG_PRODUCTION }}
CPLN_ORG_PRODUCTION: ${{ steps.cpln-orgs.outputs.production }}
shell: bash
run: |
set -euo pipefail
Expand Down
Loading