Skip to content

Commit 322b16b

Browse files
ci(deploy-camunda): fix path-traversal in extra-values validator and log leak
Address copilot review findings on #6429: - registry_validator.go: reject relative extra-values paths that escape chart-full-setup via `..` traversal (filepath.Rel guard). Add TestRegistryValidatorRejectsExtraValuesPathTraversal to pin this. - test-integration-runner.yaml: replace all four `tee /tmp/extra-values-file.yaml` instances with a plain redirect to avoid printing potentially sensitive values content into workflow logs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent cd65ba1 commit 322b16b

3 files changed

Lines changed: 31 additions & 13 deletions

File tree

.github/workflows/test-integration-runner.yaml

Lines changed: 9 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -528,8 +528,7 @@ jobs:
528528
ENTRA_APP_OBJECT_ID: ${{ steps.setup.outputs.ENTRA_APP_OBJECT_ID }}
529529
ENTRA_APP_DIRECTORY_ID: ${{ steps.setup.outputs.ENTRA_APP_DIRECTORY_ID }}
530530
run: |
531-
echo "Extra values from workflow:"
532-
echo "$EXTRA_VALUES" | tee /tmp/extra-values-file.yaml
531+
echo "$EXTRA_VALUES" > /tmp/extra-values-file.yaml
533532
deploy-camunda entra update-redirect-uris \
534533
--ingress-host "$TEST_INGRESS_HOST" \
535534
--log-level info
@@ -548,11 +547,10 @@ jobs:
548547
env:
549548
EXTRA_VALUES: ${{ inputs.extra-values }}
550549
run: |
551-
# Persist user-supplied extra values for `deploy-camunda matrix run` to consume.
552-
# The runner uses a fixed location (/tmp/extra-values-file.yaml) which is added to
553-
# the helm values chain by deploy.Execute via flags.Deployment.ExtraValues.
554-
echo "Extra values from workflow:"
555-
echo "$EXTRA_VALUES" | tee /tmp/extra-values-file.yaml
550+
# Persist user-supplied extra values for `deploy-camunda matrix run` to consume
551+
# via --extra-values (flags.Deployment.ExtraValues → digest-overlay strip).
552+
# Written without echoing to stdout to avoid leaking sensitive values into logs.
553+
echo "$EXTRA_VALUES" > /tmp/extra-values-file.yaml
556554
557555
- name: Helm - OCI Registry Login
558556
if: inputs.helmChartVersion != ''
@@ -904,8 +902,7 @@ jobs:
904902
ENTRA_APP_OBJECT_ID: ${{ steps.setup.outputs.ENTRA_APP_OBJECT_ID }}
905903
ENTRA_APP_DIRECTORY_ID: ${{ steps.setup.outputs.ENTRA_APP_DIRECTORY_ID }}
906904
run: |
907-
echo "Extra values from workflow:"
908-
echo "$EXTRA_VALUES" | tee /tmp/extra-values-file.yaml
905+
echo "$EXTRA_VALUES" > /tmp/extra-values-file.yaml
909906
deploy-camunda entra update-redirect-uris \
910907
--ingress-host "$TEST_INGRESS_HOST" \
911908
--log-level info
@@ -1043,10 +1040,9 @@ jobs:
10431040
# `matrix run` runs upgrade as two steps and applies --extra-values to
10441041
# Step 2 only (runner_upgrade.go nils Step 1's copy), so the run-built
10451042
# image tag lands on the upgraded chart without contaminating the
1046-
# previous-version install. The tee runs here unconditionally so the file
1047-
# exists regardless of auth type (the OIDC configure step also tees it).
1048-
echo "Extra values from workflow:"
1049-
echo "$EXTRA_VALUES" | tee /tmp/extra-values-file.yaml
1043+
# previous-version install. Written without echoing to stdout to avoid
1044+
# leaking potentially sensitive values into workflow logs.
1045+
echo "$EXTRA_VALUES" > /tmp/extra-values-file.yaml
10501046
EXTRA_VALUES_ARGS=()
10511047
if [[ -s /tmp/extra-values-file.yaml ]] && grep -q '[^[:space:]]' /tmp/extra-values-file.yaml; then
10521048
EXTRA_VALUES_ARGS=(--extra-values /tmp/extra-values-file.yaml)

scripts/deploy-camunda/matrix/registry_test.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -438,6 +438,22 @@ func TestRegistryValidatorRejectsMissingExtraValues(t *testing.T) {
438438
}
439439
}
440440

441+
// TestRegistryValidatorRejectsExtraValuesPathTraversal pins the traversal
442+
// guard in checkExtraValues: a relative path that escapes chart-full-setup
443+
// via `..` must be rejected even if the target file exists on disk.
444+
func TestRegistryValidatorRejectsExtraValuesPathTraversal(t *testing.T) {
445+
abs := absChartDir(t)
446+
cfg, err := LoadRegistry(abs)
447+
if err != nil {
448+
t.Fatalf("LoadRegistry: %v", err)
449+
}
450+
cfg.Integration.Case.PR.Scenarios[0].ExtraValues = []string{"../../etc/passwd"}
451+
err = (&RegistryValidator{ChartDir: abs}).Validate(cfg)
452+
if err == nil || !strings.Contains(err.Error(), "escapes chart-full-setup") {
453+
t.Fatalf("want path-traversal rejection, got: %v", err)
454+
}
455+
}
456+
441457
// TestRegistryValidatorRejectsMissingDepValuesFile exercises checkDep.
442458
// values-file paths are repo-root-relative (matching runner.go:1742); a
443459
// dangling reference must be caught at validation, not at deploy time.

scripts/deploy-camunda/matrix/registry_validator.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,12 @@ func (v *RegistryValidator) Validate(cfg *CITestConfig) error {
102102
return
103103
}
104104
path := filepath.Join(chartFullSetupDir, ev)
105+
// Reject paths that escape chart-full-setup via `..` traversal.
106+
rel, err := filepath.Rel(chartFullSetupDir, path)
107+
if err != nil || strings.HasPrefix(rel, "..") {
108+
problems = append(problems, fmt.Sprintf("%s: extra-values %q: path escapes chart-full-setup dir", ctx, ev))
109+
return
110+
}
105111
if info, err := os.Stat(path); err != nil || info.IsDir() {
106112
problems = append(problems, fmt.Sprintf("%s: extra-values %q: missing values file at %s", ctx, ev, path))
107113
}

0 commit comments

Comments
 (0)