fix: normalize secrets consistently, display accurate help (#11052)

Forgejo's UI claims that whitespace is removed from the beginning and the end of the values of Forgejo Actions variables and secrets. However, that is not correct. The entered values are stored as-is. Only CRLF is replaced with LF, which is also the desired behaviour.

This PR changes the incorrect text which is also no longer displayed as placeholder but as a proper help text below the input fields. Furthermore, tests were added to verify the behaviour.

While adding tests, I discovered and fixed another inconsistency. Depending on whether secrets were managed using the UI or the HTTP API, they were treated differently. CRLF in secrets entered in the UI was correctly replaced with LF while secrets created using the HTTP API kept CRLF.

Fixes #11003.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11052
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Co-authored-by: Andreas Ahlenstorf <andreas@ahlenstorf.ch>
Co-committed-by: Andreas Ahlenstorf <andreas@ahlenstorf.ch>

models/forgejo_migrations_legacy/v39.go

@@ -58,7 +58,7 @@ func MigrateActionSecretsToKeying(x *xorm.Engine) error {
 				return nil
 			}
 
-			bean.SetSecret(secretBytes)
+			bean.SetData(secretBytes)
 			_, err = sess.Cols("data").ID(bean.ID).Update(bean)
 			return err
 		})

models/secret/secret.go

@@ -75,7 +75,7 @@ func InsertEncryptedSecret(ctx context.Context, ownerID, repoID int64, name, dat
 			return err
 		}
 
-		secret.SetSecret(data)
+		secret.SetData(data)
 		_, err := db.GetEngine(ctx).ID(secret.ID).Cols("data").Update(secret)
 		return err
 	})
@@ -114,8 +114,19 @@ func (opts FindSecretsOptions) ToConds() builder.Cond {
 	return cond
 }
 
-func (s *Secret) SetSecret(data string) {
-	s.Data = keying.ActionSecret.Encrypt([]byte(data), keying.ColumnAndID("data", s.ID))
+func (s *Secret) SetData(data string) {
+	normalizedData := util.ReserveLineBreakForTextarea(data)
+	s.Data = keying.ActionSecret.Encrypt([]byte(normalizedData), keying.ColumnAndID("data", s.ID))
+}
+
+func (s *Secret) GetDecryptedData() (string, error) {
+	key := keying.ActionSecret
+	v, err := key.Decrypt(s.Data, keying.ColumnAndID("data", s.ID))
+	if err != nil {
+		return "", fmt.Errorf("unable to decrypt secret[id=%d,name=%q]: %w", s.ID, s.Name, err)
+	}
+
+	return string(v), nil
 }
 
 func FetchActionSecrets(ctx context.Context, ownerID, repoID int64) (map[string]string, error) {
@@ -132,14 +143,13 @@ func FetchActionSecrets(ctx context.Context, ownerID, repoID int64) (map[string]
 		return nil, err
 	}
 
-	key := keying.ActionSecret
 	for _, secret := range append(ownerSecrets, repoSecrets...) {
-		v, err := key.Decrypt(secret.Data, keying.ColumnAndID("data", secret.ID))
+		decryptedData, err := secret.GetDecryptedData()
 		if err != nil {
-			log.Error("unable to decrypt secret[id=%d,name=%q]: %v", secret.ID, secret.Name, err)
+			log.Error("%v", err)
 			return nil, err
 		}
-		secrets[secret.Name] = string(v)
+		secrets[secret.Name] = decryptedData
 	}
 
 	return secrets, nil

models/secret/secret_test.go

@@ -90,3 +90,36 @@ func TestInsertEncryptedSecret(t *testing.T) {
 		assert.Equal(t, "some repository secret", secrets["REPO_SECRET"])
 	})
 }
+
+func TestSecretDataIsNormalized(t *testing.T) {
+	secret := Secret{ID: 494, OwnerID: 829, RepoID: 0, Name: "A_SECRET"}
+
+	secret.SetData("  \r\ndatà\t  ")
+
+	decryptedData, err := secret.GetDecryptedData()
+	require.NoError(t, err)
+	assert.Equal(t, "  \ndatà\t  ", decryptedData)
+}
+
+func TestSecretGetDecryptedData(t *testing.T) {
+	t.Run("Recovers original data", func(t *testing.T) {
+		secret := Secret{ID: 494, OwnerID: 829, RepoID: 0, Name: "A_SECRET"}
+		secret.SetData("data")
+
+		decryptedData, err := secret.GetDecryptedData()
+		require.NoError(t, err)
+		assert.Equal(t, "data", decryptedData)
+	})
+
+	t.Run("Returns error if data cannot be decrypted", func(t *testing.T) {
+		secret := Secret{ID: 494, OwnerID: 829, RepoID: 0, Name: "A_SECRET"}
+		secret.SetData("data")
+
+		// Changing the ID without updating the secret makes the secret irrecoverable.
+		secret.ID++
+
+		decryptedData, err := secret.GetDecryptedData()
+		assert.Empty(t, decryptedData)
+		assert.ErrorContains(t, err, "unable to decrypt secret[id=495,name=\"A_SECRET\"]")
+	})
+}