Improve secret variables error on software upload. (#25052)

ee/server/service/maintained_apps.go

@@ -44,7 +44,17 @@ func (svc *Service) AddFleetMaintainedApp(
 	}
 
 	if err := svc.ds.ValidateEmbeddedSecrets(ctx, []string{installScript, postInstallScript, uninstallScript}); err != nil {
-		return 0, ctxerr.Wrap(ctx, fleet.NewInvalidArgumentError("script", err.Error()))
+		// We redo the validation on each script to find out which script has the missing secret.
+		// This is done to provide a more informative error message to the UI user.
+		var argErr *fleet.InvalidArgumentError
+		argErr = svc.validateEmbeddedSecretsOnScript(ctx, "install script", &installScript, argErr)
+		argErr = svc.validateEmbeddedSecretsOnScript(ctx, "post-install script", &postInstallScript, argErr)
+		argErr = svc.validateEmbeddedSecretsOnScript(ctx, "uninstall script", &uninstallScript, argErr)
+		if argErr != nil {
+			return 0, argErr
+		}
+		// We should not get to this point. If we did, it means we have another issue, such as large read replica latency.
+		return 0, ctxerr.Wrap(ctx, err, "transient server issue validating embedded secrets")
 	}
 
 	app, err := svc.ds.GetMaintainedAppByID(ctx, appID)

ee/server/service/software_installers.go

@@ -96,7 +96,17 @@ func (svc *Service) UploadSoftwareInstaller(ctx context.Context, payload *fleet.
 	preProcessUninstallScript(payload)
 
 	if err := svc.ds.ValidateEmbeddedSecrets(ctx, []string{payload.InstallScript, payload.PostInstallScript, payload.UninstallScript}); err != nil {
-		return ctxerr.Wrap(ctx, fleet.NewInvalidArgumentError("script", err.Error()))
+		// We redo the validation on each script to find out which script has the missing secret.
+		// This is done to provide a more informative error message to the UI user.
+		var argErr *fleet.InvalidArgumentError
+		argErr = svc.validateEmbeddedSecretsOnScript(ctx, "install script", &payload.InstallScript, argErr)
+		argErr = svc.validateEmbeddedSecretsOnScript(ctx, "post-install script", &payload.PostInstallScript, argErr)
+		argErr = svc.validateEmbeddedSecretsOnScript(ctx, "uninstall script", &payload.UninstallScript, argErr)
+		if argErr != nil {
+			return argErr
+		}
+		// We should not get to this point. If we did, it means we have another issue, such as large read replica latency.
+		return ctxerr.Wrap(ctx, err, "transient server issue validating embedded secrets")
 	}
 
 	installerID, titleID, err := svc.ds.MatchOrCreateSoftwareInstaller(ctx, payload)
@@ -228,7 +238,17 @@ func (svc *Service) UpdateSoftwareInstaller(ctx context.Context, payload *fleet.
 	}
 
 	if err := svc.ds.ValidateEmbeddedSecrets(ctx, scripts); err != nil {
-		return nil, ctxerr.Wrap(ctx, fleet.NewInvalidArgumentError("script", err.Error()))
+		// We redo the validation on each script to find out which script has the missing secret.
+		// This is done to provide a more informative error message to the UI user.
+		var argErr *fleet.InvalidArgumentError
+		argErr = svc.validateEmbeddedSecretsOnScript(ctx, "install script", payload.InstallScript, argErr)
+		argErr = svc.validateEmbeddedSecretsOnScript(ctx, "post-install script", payload.PostInstallScript, argErr)
+		argErr = svc.validateEmbeddedSecretsOnScript(ctx, "uninstall script", payload.UninstallScript, argErr)
+		if argErr != nil {
+			return nil, argErr
+		}
+		// We should not get to this point. If we did, it means we have another issue, such as large read replica latency.
+		return nil, ctxerr.Wrap(ctx, err, "transient server issue validating embedded secrets")
 	}
 
 	// get software by ID, fail if it does not exist or does not have an existing installer
@@ -463,6 +483,20 @@ func (svc *Service) UpdateSoftwareInstaller(ctx context.Context, payload *fleet.
 	return updatedInstaller, nil
 }
 
+func (svc *Service) validateEmbeddedSecretsOnScript(ctx context.Context, scriptName string, script *string,
+	argErr *fleet.InvalidArgumentError) *fleet.InvalidArgumentError {
+	if script != nil {
+		if errScript := svc.ds.ValidateEmbeddedSecrets(ctx, []string{*script}); errScript != nil {
+			if argErr != nil {
+				argErr.Append(scriptName, errScript.Error())
+			} else {
+				argErr = fleet.NewInvalidArgumentError(scriptName, errScript.Error())
+			}
+		}
+	}
+	return argErr
+}
+
 func ValidateSoftwareLabelsForUpdate(ctx context.Context, svc fleet.Service, existingInstaller *fleet.SoftwareInstaller, includeAny, excludeAny []string) (shouldUpdate bool, validatedLabels *fleet.LabelIdentsWithScope, err error) {
 	if authctx, ok := authz_ctx.FromContext(ctx); !ok {
 		return false, nil, fleet.NewAuthRequiredError("batch validate labels: missing authorization context")

server/service/base_client_errors.go

@@ -104,17 +104,40 @@ type serverError struct {
 }
 
 func extractServerErrorText(body io.Reader) string {
+	_, reason := extractServerErrorNameReason(body)
+	return reason
+}
+
+func extractServerErrorNameReason(body io.Reader) (string, string) {
 	var serverErr serverError
 	if err := json.NewDecoder(body).Decode(&serverErr); err != nil {
-		return "unknown"
+		return "", "unknown"
 	}
 
-	errText := serverErr.Message
+	errName := ""
+	errReason := serverErr.Message
 	if len(serverErr.Errors) > 0 {
-		errText += ": " + serverErr.Errors[0].Reason
+		errReason += ": " + serverErr.Errors[0].Reason
+		errName = serverErr.Errors[0].Name
+	}
+
+	return errName, errReason
+}
+
+func extractServerErrorNameReasons(body io.Reader) ([]string, []string) {
+	var serverErr serverError
+	if err := json.NewDecoder(body).Decode(&serverErr); err != nil {
+		return []string{""}, []string{"unknown"}
+	}
+
+	var errName []string
+	var errReason []string
+	for _, err := range serverErr.Errors {
+		errName = append(errName, err.Name)
+		errReason = append(errReason, err.Reason)
 	}
 
-	return errText
+	return errName, errReason
 }
 
 type statusCodeErr struct {

server/service/integration_enterprise_test.go

@@ -8930,15 +8930,17 @@ func (s *integrationEnterpriseTestSuite) TestAllSoftwareTitles() {
 		PostInstallScript: "d",
 		SelfService:       true,
 	}
-	s.uploadSoftwareInstaller(t, payloadEmacsMissingSecret, http.StatusUnprocessableEntity, "$FLEET_SECRET_INVALID")
+	s.uploadSoftwareInstallerWithErrorNameReason(t, payloadEmacsMissingSecret, http.StatusUnprocessableEntity, "$FLEET_SECRET_INVALID",
+		"install script")
 
 	payloadEmacsMissingPostSecret := &fleet.UploadSoftwareInstallerPayload{
 		InstallScript:     "install",
 		Filename:          "emacs.deb",
 		PostInstallScript: "d $FLEET_SECRET_INVALID",
 		SelfService:       true,
 	}
-	s.uploadSoftwareInstaller(t, payloadEmacsMissingPostSecret, http.StatusUnprocessableEntity, "$FLEET_SECRET_INVALID")
+	s.uploadSoftwareInstallerWithErrorNameReason(t, payloadEmacsMissingPostSecret, http.StatusUnprocessableEntity, "$FLEET_SECRET_INVALID",
+		"post-install script")
 
 	payloadEmacsMissingUnSecret := &fleet.UploadSoftwareInstallerPayload{
 		InstallScript:     "install",
@@ -8947,7 +8949,8 @@ func (s *integrationEnterpriseTestSuite) TestAllSoftwareTitles() {
 		UninstallScript:   "delet $FLEET_SECRET_INVALID",
 		SelfService:       true,
 	}
-	s.uploadSoftwareInstaller(t, payloadEmacsMissingUnSecret, http.StatusUnprocessableEntity, "$FLEET_SECRET_INVALID")
+	s.uploadSoftwareInstallerWithErrorNameReason(t, payloadEmacsMissingUnSecret, http.StatusUnprocessableEntity, "$FLEET_SECRET_INVALID",
+		"uninstall script")
 
 	payloadEmacs := &fleet.UploadSoftwareInstallerPayload{
 		InstallScript: "install",
@@ -15943,10 +15946,42 @@ func (s *integrationEnterpriseTestSuite) TestMaintainedApps() {
 		UninstallScript:   "echo $FLEET_SECRET_INVALID3",
 	}
 	respBadSecret := s.Do("POST", "/api/latest/fleet/software/fleet_maintained_apps", reqInvalidSecret, http.StatusUnprocessableEntity)
-	errMsg := extractServerErrorText(respBadSecret.Body)
-	require.Contains(t, errMsg, "$FLEET_SECRET_INVALID1")
-	require.Contains(t, errMsg, "$FLEET_SECRET_INVALID2")
-	require.Contains(t, errMsg, "$FLEET_SECRET_INVALID3")
+	errNames, errReasons := extractServerErrorNameReasons(respBadSecret.Body)
+	assert.ElementsMatch(t, []string{"install script", "post-install script", "uninstall script"}, errNames)
+	assert.Len(t, errReasons, 3)
+	for _, reason := range errReasons {
+		assert.Contains(t, reason, "$FLEET_SECRET_INVALID")
+	}
+
+	reqInvalidSecret = &addFleetMaintainedAppRequest{
+		AppID:             1,
+		TeamID:            &team.ID,
+		SelfService:       true,
+		PreInstallQuery:   "SELECT 1",
+		InstallScript:     "echo foo",
+		PostInstallScript: "echo done $FLEET_SECRET_INVALID2",
+		UninstallScript:   "echo",
+	}
+	respBadSecret = s.Do("POST", "/api/latest/fleet/software/fleet_maintained_apps", reqInvalidSecret, http.StatusUnprocessableEntity)
+	errNames, errReasons = extractServerErrorNameReasons(respBadSecret.Body)
+	assert.ElementsMatch(t, []string{"post-install script"}, errNames)
+	require.Len(t, errReasons, 1)
+	assert.Contains(t, errReasons[0], "$FLEET_SECRET_INVALID2")
+
+	reqInvalidSecret = &addFleetMaintainedAppRequest{
+		AppID:             1,
+		TeamID:            &team.ID,
+		SelfService:       true,
+		PreInstallQuery:   "SELECT 1",
+		InstallScript:     "echo foo",
+		PostInstallScript: "echo done",
+		UninstallScript:   "echo $FLEET_SECRET_INVALID3",
+	}
+	respBadSecret = s.Do("POST", "/api/latest/fleet/software/fleet_maintained_apps", reqInvalidSecret, http.StatusUnprocessableEntity)
+	errNames, errReasons = extractServerErrorNameReasons(respBadSecret.Body)
+	assert.ElementsMatch(t, []string{"uninstall script"}, errNames)
+	require.Len(t, errReasons, 1)
+	assert.Contains(t, errReasons[0], "$FLEET_SECRET_INVALID3")
 
 	// Add an ingested app to the team
 	var addMAResp addFleetMaintainedAppResponse

server/service/testing_client.go

@@ -557,6 +557,16 @@ func (ts *withServer) uploadSoftwareInstaller(
 	payload *fleet.UploadSoftwareInstallerPayload,
 	expectedStatus int,
 	expectedError string,
+) {
+	ts.uploadSoftwareInstallerWithErrorNameReason(t, payload, expectedStatus, expectedError, "")
+}
+
+func (ts *withServer) uploadSoftwareInstallerWithErrorNameReason(
+	t *testing.T,
+	payload *fleet.UploadSoftwareInstallerPayload,
+	expectedStatus int,
+	expectedErrorReason string,
+	expectedErrorName string,
 ) {
 	t.Helper()
 
@@ -621,9 +631,14 @@ func (ts *withServer) uploadSoftwareInstaller(
 	r := ts.DoRawWithHeaders("POST", "/api/latest/fleet/software/package", b.Bytes(), expectedStatus, headers)
 	defer r.Body.Close()
 
-	if expectedError != "" {
-		errMsg := extractServerErrorText(r.Body)
-		require.Contains(t, errMsg, expectedError)
+	if expectedErrorReason != "" || expectedErrorName != "" {
+		errName, errReason := extractServerErrorNameReason(r.Body)
+		if expectedErrorName != "" {
+			require.Equal(t, expectedErrorName, errName)
+		}
+		if expectedErrorReason != "" {
+			require.Contains(t, errReason, expectedErrorReason)
+		}
 	}
 }