Scope package identifier validation to template substitution (#41028)

Fixes #41009

## Summary

- Scope `ValidatePackageIdentifiers` to only run when `$PACKAGE_ID` or
`$UPGRADE_CODE` template variables are present in the uninstall script
  - Move `dmg`/`zip` early return before validation
- Switch from ASCII allowlist to shell metacharacter denylist, allowing
legitimate non-ASCII product names (e.g., `®`, parens) while still
blocking injection characters

  ## Test plan

- [x] Added unit tests for conditional validation (non-ASCII IDs
with/without template vars, dmg/zip bypass, upgrade code scoping)
  - [x] Existing input tests still pass
  - [x] Winget ingester tests unaffected

---------

Co-authored-by: Ian Littman <iansltx@gmail.com>

ee/server/service/software_installers.go

@@ -238,41 +238,48 @@ func ValidateSoftwareLabels(ctx context.Context, svc fleet.Service, teamID *uint
 }
 
 func preProcessUninstallScript(payload *fleet.UploadSoftwareInstallerPayload) error {
-	// We assume that we already validated that payload.PackageIDs is not empty.
-	// Replace $PACKAGE_ID in the uninstall script with the package ID(s).
 	if len(payload.PackageIDs) == 0 {
 		// do nothing, this could be a FMA which won't include the installer when editing the scripts
 		return nil
 	}
 
-	// Validate that package identifiers and upgrade codes don't contain shell metacharacters
-	if err := file.ValidatePackageIdentifiers(payload.PackageIDs, payload.UpgradeCode); err != nil {
-		return err
-	}
-
-	var packageID string
+	// dmg and zip don't use template variable substitution
 	switch payload.Extension {
 	case "dmg", "zip":
 		return nil
-	case "pkg":
-		var sb strings.Builder
-		_, _ = sb.WriteString("(\n")
-		for _, pkgID := range payload.PackageIDs {
-			_, _ = sb.WriteString(fmt.Sprintf("  '%s'\n", pkgID))
-		}
-		_, _ = sb.WriteString(")") // no ending newline
-		packageID = sb.String()
-	default:
-		packageID = fmt.Sprintf("'%s'", payload.PackageIDs[0])
 	}
 
-	payload.UninstallScript = file.PackageIDRegex.ReplaceAllString(payload.UninstallScript, fmt.Sprintf("%s${suffix}", packageID))
+	// Only validate and substitute $PACKAGE_ID if it appears in the script
+	if file.PackageIDRegex.MatchString(payload.UninstallScript) {
+		if err := file.ValidatePackageIdentifiers(payload.PackageIDs, ""); err != nil {
+			return err
+		}
 
-	// If $UPGRADE_CODE is in the script and we have one, replace it; if the var is included but we don't have one, error
+		var packageID string
+		switch payload.Extension {
+		case "pkg":
+			var sb strings.Builder
+			_, _ = sb.WriteString("(\n")
+			for _, pkgID := range payload.PackageIDs {
+				_, _ = sb.WriteString(fmt.Sprintf("  '%s'\n", pkgID))
+			}
+			_, _ = sb.WriteString(")") // no ending newline
+			packageID = sb.String()
+		default:
+			packageID = fmt.Sprintf("'%s'", payload.PackageIDs[0])
+		}
+
+		payload.UninstallScript = file.PackageIDRegex.ReplaceAllString(payload.UninstallScript, fmt.Sprintf("%s${suffix}", packageID))
+	}
+
+	// Only validate and substitute $UPGRADE_CODE if the template variable appears in the script
 	if file.UpgradeCodeRegex.MatchString(payload.UninstallScript) {
 		if payload.UpgradeCode == "" {
 			return errors.New("$UPGRADE_CODE variable was used in uninstall script but package does not have an UpgradeCode")
 		}
+		if err := file.ValidatePackageIdentifiers(nil, payload.UpgradeCode); err != nil {
+			return err
+		}
 
 		payload.UninstallScript = file.UpgradeCodeRegex.ReplaceAllString(payload.UninstallScript, fmt.Sprintf("'%s'${suffix}", payload.UpgradeCode))
 	}

ee/server/service/software_installers_test.go

@@ -150,6 +150,85 @@ func TestPreProcessUninstallScriptMaliciousInput(t *testing.T) {
 	}
 }
 
+func TestPreProcessUninstallScriptSkipsValidationWhenNoTemplateVars(t *testing.T) {
+	t.Parallel()
+
+	// Non-ASCII package ID that would fail the safeIdentifierRegex validation
+	nonASCIIID := "CrossCore\u00ae Embedded Studio v3.0.2"
+
+	t.Run("non-ASCII ID succeeds when script has no template vars", func(t *testing.T) {
+		payload := fleet.UploadSoftwareInstallerPayload{
+			Extension:       "exe",
+			UninstallScript: `$softwareName = "CrossCore Embedded Studio"`,
+			PackageIDs:      []string{nonASCIIID},
+		}
+		require.NoError(t, preProcessUninstallScript(&payload))
+		assert.Equal(t, `$softwareName = "CrossCore Embedded Studio"`, payload.UninstallScript)
+	})
+
+	t.Run("non-ASCII ID succeeds when script uses PACKAGE_ID", func(t *testing.T) {
+		payload := fleet.UploadSoftwareInstallerPayload{
+			Extension:       "exe",
+			UninstallScript: "$PACKAGE_ID",
+			PackageIDs:      []string{nonASCIIID},
+		}
+		require.NoError(t, preProcessUninstallScript(&payload))
+		assert.Contains(t, payload.UninstallScript, "'"+nonASCIIID+"'")
+	})
+
+	t.Run("non-ASCII upgrade code succeeds when script has no UPGRADE_CODE", func(t *testing.T) {
+		payload := fleet.UploadSoftwareInstallerPayload{
+			Extension:       "msi",
+			UninstallScript: "msiexec /x $PACKAGE_ID /quiet",
+			PackageIDs:      []string{"valid-id"},
+			UpgradeCode:     "code\u00ae",
+		}
+		require.NoError(t, preProcessUninstallScript(&payload))
+		assert.Contains(t, payload.UninstallScript, "'valid-id'")
+	})
+
+	t.Run("non-ASCII upgrade code succeeds when script uses UPGRADE_CODE", func(t *testing.T) {
+		payload := fleet.UploadSoftwareInstallerPayload{
+			Extension:       "msi",
+			UninstallScript: "msiexec /x $UPGRADE_CODE /quiet",
+			PackageIDs:      []string{"valid-id"},
+			UpgradeCode:     "code\u00ae",
+		}
+		require.NoError(t, preProcessUninstallScript(&payload))
+		assert.Contains(t, payload.UninstallScript, "'code\u00ae'")
+	})
+
+	t.Run("dmg skips validation entirely", func(t *testing.T) {
+		payload := fleet.UploadSoftwareInstallerPayload{
+			Extension:       "dmg",
+			UninstallScript: "$PACKAGE_ID\n\necho 'foo'",
+			PackageIDs:      []string{nonASCIIID},
+		}
+		require.NoError(t, preProcessUninstallScript(&payload))
+		require.Equal(t, "$PACKAGE_ID\n\necho 'foo'", payload.UninstallScript) // confirm no variable substitution
+	})
+
+	t.Run("zip skips validation entirely", func(t *testing.T) {
+		payload := fleet.UploadSoftwareInstallerPayload{
+			Extension:       "zip",
+			UninstallScript: "$PACKAGE_ID\n\necho 'foo'",
+			PackageIDs:      []string{nonASCIIID},
+		}
+		require.NoError(t, preProcessUninstallScript(&payload))
+		require.Equal(t, "$PACKAGE_ID\n\necho 'foo'", payload.UninstallScript) // confirm no variable substitution
+	})
+
+	t.Run("empty PackageIDs skips processing", func(t *testing.T) {
+		payload := fleet.UploadSoftwareInstallerPayload{
+			Extension:       "exe",
+			UninstallScript: "$PACKAGE_ID",
+			PackageIDs:      []string{},
+		}
+		require.NoError(t, preProcessUninstallScript(&payload))
+		require.Equal(t, "$PACKAGE_ID", payload.UninstallScript) // confirm no variable substitution
+	})
+}
+
 func TestInstallUninstallAuth(t *testing.T) {
 	t.Parallel()
 	ds := new(mock.Store)

pkg/file/management.go

@@ -80,22 +80,18 @@ var UninstallMsiWithUpgradeCodeScript string
 var PackageIDRegex = regexp.MustCompile(`((("\$PACKAGE_ID")|(\$PACKAGE_ID))(?P<suffix>\W|$))|(("\${PACKAGE_ID}")|(\${PACKAGE_ID}))`)
 var UpgradeCodeRegex = regexp.MustCompile(`((("\$UPGRADE_CODE")|(\$UPGRADE_CODE))(?P<suffix>\W|$))|(("\${UPGRADE_CODE}")|(\${UPGRADE_CODE}))`)
 
-// safeIdentifierRegex matches strings that contain only safe characters for
-// interpolation into shell scripts. This allowlist prevents shell injection
-// via crafted package metadata (e.g., package IDs containing $(), backticks,
-// pipes, or other shell metacharacters).
-var safeIdentifierRegex = regexp.MustCompile(`^[a-zA-Z0-9._\-{} +,/:~@]+$`)
+// shellMetacharRegex matches shell metacharacters that are unsafe for script interpolation.
+var shellMetacharRegex = regexp.MustCompile("['" + `"` + "`" + `$\\|;&><!\n\r]`)
 
-// ValidatePackageIdentifiers checks that package IDs and upgrade codes contain
-// only safe characters for shell script interpolation. Returns an error if any
-// identifier contains shell metacharacters.
+// ValidatePackageIdentifiers checks that package IDs and upgrade codes do not
+// contain shell metacharacters.
 func ValidatePackageIdentifiers(packageIDs []string, upgradeCode string) error {
 	for _, id := range packageIDs {
-		if !safeIdentifierRegex.MatchString(id) {
+		if shellMetacharRegex.MatchString(id) {
 			return fmt.Errorf("package identifier %q contains invalid characters", id)
 		}
 	}
-	if upgradeCode != "" && !safeIdentifierRegex.MatchString(upgradeCode) {
+	if upgradeCode != "" && shellMetacharRegex.MatchString(upgradeCode) {
 		return fmt.Errorf("upgrade code %q contains invalid characters", upgradeCode)
 	}
 	return nil

pkg/file/management_test.go

@@ -81,6 +81,10 @@ func TestValidatePackageIdentifiers(t *testing.T) {
 			"a~b",
 			"comma,separated",
 			"with spaces",
+			"CrossCore\u00ae Embedded Studio v3.0.2",
+			"Adobe Acrobat (64-bit)",
+			"C# Runtime",
+			"日本語アプリ",
 		}
 		require.NoError(t, ValidatePackageIdentifiers(validIDs, ""))
 		require.NoError(t, ValidatePackageIdentifiers(nil, "{UPGRADE-CODE-123}"))