From f7873ba39368e1d84f05b66aadc9a7151c02f495 Mon Sep 17 00:00:00 2001 From: Andreas Ahlenstorf Date: Mon, 9 Feb 2026 17:02:18 +0100 Subject: [PATCH] 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 Co-authored-by: Andreas Ahlenstorf Co-committed-by: Andreas Ahlenstorf --- models/forgejo_migrations_legacy/v39.go | 2 +- models/secret/secret.go | 24 ++- models/secret/secret_test.go | 33 ++++ modules/structs/secret.go | 5 +- modules/structs/variable.go | 13 +- options/locale/locale_en-US.ini | 2 - options/locale_next/locale_en-US.json | 4 + services/secrets/secrets.go | 2 +- templates/shared/secrets/add_list.tmpl | 8 +- templates/shared/variables/variable_list.tmpl | 8 +- templates/swagger/v1_json.tmpl | 14 +- tests/integration/actions_secrets_test.go | 174 ++++++++++++++++++ tests/integration/actions_variables_test.go | 6 +- tests/integration/api_org_secrets_test.go | 29 ++- tests/integration/api_org_variables_test.go | 137 ++++++++------ tests/integration/api_repo_secrets_test.go | 33 ++-- tests/integration/api_repo_variables_test.go | 151 +++++++++------ tests/integration/api_user_secrets_test.go | 28 +-- tests/integration/api_user_variables_test.go | 147 +++++++++------ 19 files changed, 586 insertions(+), 234 deletions(-) create mode 100644 tests/integration/actions_secrets_test.go diff --git a/models/forgejo_migrations_legacy/v39.go b/models/forgejo_migrations_legacy/v39.go index 3f0d94b6aa..616404f7eb 100644 --- a/models/forgejo_migrations_legacy/v39.go +++ b/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 }) diff --git a/models/secret/secret.go b/models/secret/secret.go index b19199c177..de1a8d84a4 100644 --- a/models/secret/secret.go +++ b/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 diff --git a/models/secret/secret_test.go b/models/secret/secret_test.go index 50120ecfa3..e49100eb6a 100644 --- a/models/secret/secret_test.go +++ b/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\"]") + }) +} diff --git a/modules/structs/secret.go b/modules/structs/secret.go index a0673ca08c..acd5c36f06 100644 --- a/modules/structs/secret.go +++ b/modules/structs/secret.go @@ -14,10 +14,11 @@ type Secret struct { Created time.Time `json:"created_at"` } -// CreateOrUpdateSecretOption options when creating or updating secret +// CreateOrUpdateSecretOption defines the properties of the secret to create or update. // swagger:model type CreateOrUpdateSecretOption struct { - // Data of the secret to update + // Data of the secret. Special characters will be retained. Line endings will be normalized to LF to match the + // behaviour of browsers. Encode the data with Base64 if line endings should be retained. // // required: true Data string `json:"data" binding:"Required"` diff --git a/modules/structs/variable.go b/modules/structs/variable.go index cc846cf0ec..c88aad5138 100644 --- a/modules/structs/variable.go +++ b/modules/structs/variable.go @@ -3,21 +3,24 @@ package structs -// CreateVariableOption the option when creating variable +// CreateVariableOption defines the properties of the variable to create. // swagger:model type CreateVariableOption struct { - // Value of the variable to create + // Value of the variable to create. Special characters will be retained. Line endings will be normalized to LF to + // match the behaviour of browsers. Encode the data with Base64 if line endings should be retained. // // required: true Value string `json:"value" binding:"Required"` } -// UpdateVariableOption the option when updating variable +// UpdateVariableOption defines the properties of the variable to update. // swagger:model type UpdateVariableOption struct { - // New name for the variable. If the field is empty, the variable name won't be updated. + // New name for the variable. If the field is empty, the variable name won't be updated. Forgejo will convert it to + // uppercase. Name string `json:"name"` - // Value of the variable to update + // Value of the variable to update. Special characters will be retained. Line endings will be normalized to LF to + // match the behaviour of browsers. Encode the data with Base64 if line endings should be retained. // // required: true Value string `json:"value" binding:"Required"` diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 00e4adb378..8f338f238b 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -3550,8 +3550,6 @@ secrets = Secrets description = Secrets will be passed to certain actions and cannot be read otherwise. none = There are no secrets yet. creation = Add secret -creation.name_placeholder = case-insensitive, alphanumeric characters or underscores only, cannot start with GITEA_ or GITHUB_ -creation.value_placeholder = Input any content. Whitespace at the start and end will be omitted. creation.success = The secret "%s" has been added. creation.failed = Failed to add secret. deletion = Remove secret diff --git a/options/locale_next/locale_en-US.json b/options/locale_next/locale_en-US.json index 2778c44bae..c25e6beff4 100644 --- a/options/locale_next/locale_en-US.json +++ b/options/locale_next/locale_en-US.json @@ -401,6 +401,10 @@ "actions.workflow.incomplete_with_missing_matrix_dimension": "Unable to evaluate `with` of job %[1]s: matrix dimension %[2]s does not exist.", "actions.workflow.incomplete_with_unknown_cause": "Unable to evaluate `with` of job %[1]s: unknown error.", "actions.workflow.pre_execution_error": "Workflow was not executed due to an error that blocked the execution attempt.", + "actions.secrets.creation.name_description": "The name of a secret can only contain letters, numbers, and underscores. It cannot start with FORGEJO_, GITEA_, GITHUB_, or a number. Forgejo will automatically convert it to uppercase.", + "actions.secrets.creation.value_description": "The value of a secret can be any text. Special characters are retained. CRLF (Windows-style line breaks) is automatically converted to LF. Encode the value with Base64 if linebreaks should be retained.", + "actions.variables.mutation.name_description": "The name of a variable can only contain letters, numbers, and underscores. It cannot be named CI or start with FORGEJO_, GITEA_, GITHUB_, or a number. Forgejo will automatically convert it to uppercase.", + "actions.variables.mutation.value_description": "A variable's value can be any text. Special characters are retained. CRLF (Windows-style line breaks) is automatically converted to LF. Encode the value with Base64 if linebreaks should be retained.", "pulse.n_active_issues": { "one": "%s active issue", "other": "%s active issues" diff --git a/services/secrets/secrets.go b/services/secrets/secrets.go index 212016cf1e..498f3b6fcc 100644 --- a/services/secrets/secrets.go +++ b/services/secrets/secrets.go @@ -32,7 +32,7 @@ func CreateOrUpdateSecret(ctx context.Context, ownerID, repoID int64, name, data return s, true, nil } - s.SetSecret(data) + s.SetData(data) if _, err := db.GetEngine(ctx).Cols("data").ID(s.ID).Update(s); err != nil { return nil, false, err } diff --git a/templates/shared/secrets/add_list.tmpl b/templates/shared/secrets/add_list.tmpl index d4f3744717..2aa9534771 100644 --- a/templates/shared/secrets/add_list.tmpl +++ b/templates/shared/secrets/add_list.tmpl @@ -52,7 +52,7 @@
-
+
{{ctx.Locale.Tr "secrets.description"}}
@@ -63,18 +63,18 @@ name="name" value="{{.name}}" pattern="^(?!FORGEJO_|GITEA_|GITHUB_)[a-zA-Z_][a-zA-Z0-9_]*$" - placeholder="{{ctx.Locale.Tr "secrets.creation.name_placeholder"}}" > +

{{ctx.Locale.Tr "actions.secrets.creation.name_description"}}

+

{{ctx.Locale.Tr "actions.secrets.creation.value_description"}}

- + {{template "base/modal_actions_confirm" (dict "ModalButtonTypes" "confirm")}}
diff --git a/templates/shared/variables/variable_list.tmpl b/templates/shared/variables/variable_list.tmpl index ac7b0afe85..82acb66d14 100644 --- a/templates/shared/variables/variable_list.tmpl +++ b/templates/shared/variables/variable_list.tmpl @@ -62,7 +62,7 @@ + {{template "base/modal_actions_confirm" (dict "ModalButtonTypes" "confirm")}} diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 29d2683a5f..db2ca9dc13 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -24044,14 +24044,14 @@ "x-go-package": "forgejo.org/modules/structs" }, "CreateOrUpdateSecretOption": { - "description": "CreateOrUpdateSecretOption options when creating or updating secret", "type": "object", + "title": "CreateOrUpdateSecretOption defines the properties of the secret to create or update.", "required": [ "data" ], "properties": { "data": { - "description": "Data of the secret to update", + "description": "Data of the secret. Special characters will be retained. Line endings will be normalized to LF to match the\nbehaviour of browsers. Encode the data with Base64 if line endings should be retained.", "type": "string", "x-go-name": "Data" } @@ -24605,14 +24605,14 @@ "x-go-package": "forgejo.org/modules/structs" }, "CreateVariableOption": { - "description": "CreateVariableOption the option when creating variable", "type": "object", + "title": "CreateVariableOption defines the properties of the variable to create.", "required": [ "value" ], "properties": { "value": { - "description": "Value of the variable to create", + "description": "Value of the variable to create. Special characters will be retained. Line endings will be normalized to LF to\nmatch the behaviour of browsers. Encode the data with Base64 if line endings should be retained.", "type": "string", "x-go-name": "Value" } @@ -29695,19 +29695,19 @@ "x-go-package": "forgejo.org/modules/structs" }, "UpdateVariableOption": { - "description": "UpdateVariableOption the option when updating variable", "type": "object", + "title": "UpdateVariableOption defines the properties of the variable to update.", "required": [ "value" ], "properties": { "name": { - "description": "New name for the variable. If the field is empty, the variable name won't be updated.", + "description": "New name for the variable. If the field is empty, the variable name won't be updated. Forgejo will convert it to\nuppercase.", "type": "string", "x-go-name": "Name" }, "value": { - "description": "Value of the variable to update", + "description": "Value of the variable to update. Special characters will be retained. Line endings will be normalized to LF to\nmatch the behaviour of browsers. Encode the data with Base64 if line endings should be retained.", "type": "string", "x-go-name": "Value" } diff --git a/tests/integration/actions_secrets_test.go b/tests/integration/actions_secrets_test.go new file mode 100644 index 0000000000..0070c40555 --- /dev/null +++ b/tests/integration/actions_secrets_test.go @@ -0,0 +1,174 @@ +// Copyright 2026 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package integration + +import ( + "fmt" + "net/http" + "testing" + + repo_model "forgejo.org/models/repo" + secret_model "forgejo.org/models/secret" + "forgejo.org/models/unittest" + user_model "forgejo.org/models/user" + app_context "forgejo.org/services/context" + "forgejo.org/tests" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestActionsSecretsManageUserSecrets(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + url := "/user/settings/actions/secrets" + session := loginUser(t, user.Name) + + t.Run("Create secret", func(t *testing.T) { + req := NewRequestWithValues(t, "POST", url, map[string]string{ + "name": "my_secret", + "data": " \r\n\tSecrët dåtä\\ \r\n", + }) + session.MakeRequest(t, req, http.StatusOK) + + flashCookie := session.GetCookie(app_context.CookieNameFlash) + assert.NotNil(t, flashCookie) + assert.Equal(t, "success%3DThe%2Bsecret%2B%2522MY_SECRET%2522%2Bhas%2Bbeen%2Badded.", flashCookie.Value) + + secret := unittest.AssertExistsAndLoadBean(t, &secret_model.Secret{OwnerID: user.ID, RepoID: 0, Name: "MY_SECRET"}) + assert.Equal(t, "MY_SECRET", secret.Name) + + value, err := secret.GetDecryptedData() + require.NoError(t, err) + assert.Equal(t, " \n\tSecrët dåtä\\ \n", value) + }) + + t.Run("Remove secret", func(t *testing.T) { + req := NewRequestWithValues(t, "POST", url, map[string]string{ + "name": "TEST_SECRET", + "data": "value", + }) + session.MakeRequest(t, req, http.StatusOK) + + flashCookie := session.GetCookie(app_context.CookieNameFlash) + assert.NotNil(t, flashCookie) + assert.Equal(t, "success%3DThe%2Bsecret%2B%2522TEST_SECRET%2522%2Bhas%2Bbeen%2Badded.", flashCookie.Value) + + secret := unittest.AssertExistsAndLoadBean(t, &secret_model.Secret{OwnerID: user.ID, RepoID: 0, Name: "TEST_SECRET"}) + + req = NewRequest(t, "POST", fmt.Sprintf("%s/delete?id=%d", url, secret.ID)) + session.MakeRequest(t, req, http.StatusOK) + + flashCookie = session.GetCookie(app_context.CookieNameFlash) + assert.NotNil(t, flashCookie) + assert.Equal(t, "success%3DThe%2Bsecret%2Bhas%2Bbeen%2Bremoved.", flashCookie.Value) + + unittest.AssertNotExistsBean(t, secret) + }) +} + +func TestActionsSecretsManageRepositorySecrets(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1, OwnerID: user.ID}) + url := "/" + repo.FullName() + "/settings/actions/secrets" + + session := loginUser(t, user.Name) + + t.Run("Create secret", func(t *testing.T) { + req := NewRequestWithValues(t, "POST", url, map[string]string{ + "name": "my_secret", + "data": " \r\n\tSecrët dåtä\\ \r\n", + }) + session.MakeRequest(t, req, http.StatusOK) + + flashCookie := session.GetCookie(app_context.CookieNameFlash) + assert.NotNil(t, flashCookie) + assert.Equal(t, "success%3DThe%2Bsecret%2B%2522MY_SECRET%2522%2Bhas%2Bbeen%2Badded.", flashCookie.Value) + + secret := unittest.AssertExistsAndLoadBean(t, &secret_model.Secret{OwnerID: 0, RepoID: repo.ID, Name: "MY_SECRET"}) + assert.Equal(t, "MY_SECRET", secret.Name) + + value, err := secret.GetDecryptedData() + require.NoError(t, err) + assert.Equal(t, " \n\tSecrët dåtä\\ \n", value) + }) + + t.Run("Remove secret", func(t *testing.T) { + req := NewRequestWithValues(t, "POST", url, map[string]string{ + "name": "TEST_SECRET", + "data": "value", + }) + session.MakeRequest(t, req, http.StatusOK) + + flashCookie := session.GetCookie(app_context.CookieNameFlash) + assert.NotNil(t, flashCookie) + assert.Equal(t, "success%3DThe%2Bsecret%2B%2522TEST_SECRET%2522%2Bhas%2Bbeen%2Badded.", flashCookie.Value) + + secret := unittest.AssertExistsAndLoadBean(t, &secret_model.Secret{OwnerID: 0, RepoID: repo.ID, Name: "TEST_SECRET"}) + + req = NewRequest(t, "POST", fmt.Sprintf("%s/delete?id=%d", url, secret.ID)) + session.MakeRequest(t, req, http.StatusOK) + + flashCookie = session.GetCookie(app_context.CookieNameFlash) + assert.NotNil(t, flashCookie) + assert.Equal(t, "success%3DThe%2Bsecret%2Bhas%2Bbeen%2Bremoved.", flashCookie.Value) + + unittest.AssertNotExistsBean(t, secret) + }) +} + +func TestActionsSecretsManageOrganizationSecrets(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + org := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 3, Type: user_model.UserTypeOrganization}) + url := "/org/" + org.Name + "/settings/actions/secrets" + + session := loginUser(t, user.Name) + + t.Run("Create secret", func(t *testing.T) { + req := NewRequestWithValues(t, "POST", url, map[string]string{ + "name": "my_secret", + "data": " \r\n\tSecrët dåtä\\ \r\n", + }) + session.MakeRequest(t, req, http.StatusOK) + + flashCookie := session.GetCookie(app_context.CookieNameFlash) + assert.NotNil(t, flashCookie) + assert.Equal(t, "success%3DThe%2Bsecret%2B%2522MY_SECRET%2522%2Bhas%2Bbeen%2Badded.", flashCookie.Value) + + secret := unittest.AssertExistsAndLoadBean(t, &secret_model.Secret{OwnerID: org.ID, RepoID: 0, Name: "MY_SECRET"}) + assert.Equal(t, "MY_SECRET", secret.Name) + + value, err := secret.GetDecryptedData() + require.NoError(t, err) + assert.Equal(t, " \n\tSecrët dåtä\\ \n", value) + }) + + t.Run("Remove secret", func(t *testing.T) { + req := NewRequestWithValues(t, "POST", url, map[string]string{ + "name": "TEST_SECRET", + "data": "value", + }) + session.MakeRequest(t, req, http.StatusOK) + + flashCookie := session.GetCookie(app_context.CookieNameFlash) + assert.NotNil(t, flashCookie) + assert.Equal(t, "success%3DThe%2Bsecret%2B%2522TEST_SECRET%2522%2Bhas%2Bbeen%2Badded.", flashCookie.Value) + + secret := unittest.AssertExistsAndLoadBean(t, &secret_model.Secret{OwnerID: org.ID, RepoID: 0, Name: "TEST_SECRET"}) + + req = NewRequest(t, "POST", fmt.Sprintf("%s/delete?id=%d", url, secret.ID)) + session.MakeRequest(t, req, http.StatusOK) + + flashCookie = session.GetCookie(app_context.CookieNameFlash) + assert.NotNil(t, flashCookie) + assert.Equal(t, "success%3DThe%2Bsecret%2Bhas%2Bbeen%2Bremoved.", flashCookie.Value) + + unittest.AssertNotExistsBean(t, secret) + }) +} diff --git a/tests/integration/actions_variables_test.go b/tests/integration/actions_variables_test.go index 81041b350c..f609361d90 100644 --- a/tests/integration/actions_variables_test.go +++ b/tests/integration/actions_variables_test.go @@ -53,7 +53,7 @@ func TestActionsVariablesModification(t *testing.T) { req := NewRequestWithValues(t, "POST", baseURL+fmt.Sprintf("/%d/edit", id), map[string]string{ "name": "glados_quote", - "data": "I'm fine. Two plus two is...ten, in base four, I'm fine!", + "data": " \r\n\tI'm fine. Two plus two is...ten, in base four, I'm fine! \r\n", }) if fail { resp := sess.MakeRequest(t, req, http.StatusBadRequest) @@ -65,6 +65,10 @@ func TestActionsVariablesModification(t *testing.T) { flashCookie := sess.GetCookie(app_context.CookieNameFlash) assert.NotNil(t, flashCookie) assert.Equal(t, "success%3DThe%2Bvariable%2Bhas%2Bbeen%2Bedited.", flashCookie.Value) + + updatedVariable := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionVariable{ID: id}) + assert.Equal(t, "GLADOS_QUOTE", updatedVariable.Name) + assert.Equal(t, " \n\tI'm fine. Two plus two is...ten, in base four, I'm fine! \n", updatedVariable.Data) } req = NewRequest(t, "POST", baseURL+fmt.Sprintf("/%d/delete", id)) diff --git a/tests/integration/api_org_secrets_test.go b/tests/integration/api_org_secrets_test.go index 2a0574549d..02c4cfbc34 100644 --- a/tests/integration/api_org_secrets_test.go +++ b/tests/integration/api_org_secrets_test.go @@ -13,7 +13,6 @@ import ( org_model "forgejo.org/models/organization" secret_model "forgejo.org/models/secret" "forgejo.org/models/unittest" - "forgejo.org/modules/keying" api "forgejo.org/modules/structs" "forgejo.org/tests" @@ -98,10 +97,23 @@ func TestAPIOrgSecrets(t *testing.T) { } for _, c := range cases { - req := NewRequestWithJSON(t, "PUT", fmt.Sprintf("/api/v1/orgs/%s/actions/secrets/%s", org.Name, c.Name), api.CreateOrUpdateSecretOption{ - Data: "data", - }).AddTokenAuth(token) + url := fmt.Sprintf("/api/v1/orgs/%s/actions/secrets/%s", org.Name, c.Name) + + req := NewRequestWithJSON(t, "PUT", url, api.CreateOrUpdateSecretOption{ + Data: " \r\n\tdätå \r\n", + }) + req.AddTokenAuth(token) MakeRequest(t, req, c.ExpectedStatus) + + if c.ExpectedStatus < 300 { + secret := unittest.AssertExistsAndLoadBean(t, &secret_model.Secret{OwnerID: org.ID, Name: strings.ToUpper(c.Name)}) + + assert.Equal(t, strings.ToUpper(c.Name), secret.Name) + + value, err := secret.GetDecryptedData() + require.NoError(t, err) + assert.Equal(t, " \n\tdätå \n", value) + } } }) @@ -115,14 +127,15 @@ func TestAPIOrgSecrets(t *testing.T) { MakeRequest(t, req, http.StatusCreated) req = NewRequestWithJSON(t, "PUT", url, api.CreateOrUpdateSecretOption{ - Data: "changed data", - }).AddTokenAuth(token) + Data: "\r\n chåñgéd dätå\t \r\n", + }) + req.AddTokenAuth(token) MakeRequest(t, req, http.StatusNoContent) secret := unittest.AssertExistsAndLoadBean(t, &secret_model.Secret{Name: strings.ToUpper(name)}) - data, err := keying.ActionSecret.Decrypt(secret.Data, keying.ColumnAndID("data", secret.ID)) + data, err := secret.GetDecryptedData() require.NoError(t, err) - assert.Equal(t, "changed data", string(data)) + assert.Equal(t, "\n chåñgéd dätå\t \n", data) }) t.Run("Delete", func(t *testing.T) { diff --git a/tests/integration/api_org_variables_test.go b/tests/integration/api_org_variables_test.go index f3ef009772..982c2f8fa5 100644 --- a/tests/integration/api_org_variables_test.go +++ b/tests/integration/api_org_variables_test.go @@ -74,20 +74,23 @@ func TestAPIOrgVariablesCreateOrganizationVariable(t *testing.T) { for _, c := range cases { requestURL := fmt.Sprintf("/api/v1/orgs/%s/actions/variables/%s", org.Name, c.Name) + request := NewRequestWithJSON(t, "POST", requestURL, api.CreateVariableOption{ - Value: "value" + c.Name, + Value: " \tvalüé\r\n" + c.Name + " \r\n", }) request.AddTokenAuth(token) MakeRequest(t, request, c.ExpectedStatus) if c.ExpectedStatus < 300 { - request = NewRequest(t, "GET", requestURL). - AddTokenAuth(token) + request = NewRequest(t, "GET", requestURL) + request.AddTokenAuth(token) res := MakeRequest(t, request, http.StatusOK) + variable := api.ActionVariable{} DecodeJSON(t, res, &variable) + assert.Equal(t, variable.Name, c.Name) - assert.Equal(t, variable.Data, "value"+c.Name) + assert.Equal(t, variable.Data, " \tvalüé\n"+c.Name+" \n") } } } @@ -108,56 +111,84 @@ func TestAPIOrgVariablesUpdateOrganizationVariable(t *testing.T) { MakeRequest(t, request, http.StatusNoContent) - cases := []struct { - Name string - UpdateName string - ExpectedStatus int - }{ - { - Name: "not_found_var", - ExpectedStatus: http.StatusNotFound, - }, - { - Name: variableName, - UpdateName: "1invalid", - ExpectedStatus: http.StatusBadRequest, - }, - { - Name: variableName, - UpdateName: "invalid@name", - ExpectedStatus: http.StatusBadRequest, - }, - { - Name: variableName, - UpdateName: "ci", - ExpectedStatus: http.StatusBadRequest, - }, - { - Name: variableName, - UpdateName: "forgejo_foo", - ExpectedStatus: http.StatusBadRequest, - }, - { - Name: variableName, - UpdateName: "updated_var_name", - ExpectedStatus: http.StatusNoContent, - }, - { - Name: variableName, - ExpectedStatus: http.StatusNotFound, - }, - { - Name: "updated_var_name", - ExpectedStatus: http.StatusNoContent, - }, - } + t.Run("Accepts only valid names", func(t *testing.T) { + cases := []struct { + Name string + UpdateName string + ExpectedStatus int + }{ + { + Name: "not_found_var", + ExpectedStatus: http.StatusNotFound, + }, + { + Name: variableName, + UpdateName: "1invalid", + ExpectedStatus: http.StatusBadRequest, + }, + { + Name: variableName, + UpdateName: "invalid@name", + ExpectedStatus: http.StatusBadRequest, + }, + { + Name: variableName, + UpdateName: "ci", + ExpectedStatus: http.StatusBadRequest, + }, + { + Name: variableName, + UpdateName: "forgejo_foo", + ExpectedStatus: http.StatusBadRequest, + }, + { + Name: variableName, + UpdateName: "updated_var_name", + ExpectedStatus: http.StatusNoContent, + }, + { + Name: variableName, + ExpectedStatus: http.StatusNotFound, + }, + { + Name: "updated_var_name", + ExpectedStatus: http.StatusNoContent, + }, + } - for _, c := range cases { - url := fmt.Sprintf("/api/v1/orgs/%s/actions/variables/%s", org.Name, c.Name) - request := NewRequestWithJSON(t, "PUT", url, api.UpdateVariableOption{Name: c.UpdateName, Value: "updated_val"}) - request.AddTokenAuth(token) - MakeRequest(t, request, c.ExpectedStatus) - } + for _, c := range cases { + url := fmt.Sprintf("/api/v1/orgs/%s/actions/variables/%s", org.Name, c.Name) + request := NewRequestWithJSON(t, "PUT", url, api.UpdateVariableOption{Name: c.UpdateName, Value: "updated_val"}) + request.AddTokenAuth(token) + MakeRequest(t, request, c.ExpectedStatus) + } + }) + + t.Run("Retains special characters", func(t *testing.T) { + variableName := "special_characters" + url := fmt.Sprintf("/api/v1/orgs/%s/actions/variables/%s", org.Name, variableName) + + req := NewRequestWithJSON(t, "POST", url, api.CreateVariableOption{Value: "initial_value"}) + req.AddTokenAuth(token) + MakeRequest(t, req, http.StatusNoContent) + + requestData := api.UpdateVariableOption{ + Value: "\r\n \tüpdåtéd\r\n \r\n", + } + req = NewRequestWithJSON(t, "PUT", url, requestData) + req.AddTokenAuth(token) + MakeRequest(t, req, http.StatusNoContent) + + req = NewRequest(t, "GET", url) + req.AddTokenAuth(token) + res := MakeRequest(t, req, http.StatusOK) + + variable := api.ActionVariable{} + DecodeJSON(t, res, &variable) + + assert.Equal(t, "SPECIAL_CHARACTERS", variable.Name) + assert.Equal(t, "\n \tüpdåtéd\n \n", variable.Data) + }) } func TestAPIOrgVariablesDeleteOrganizationVariable(t *testing.T) { diff --git a/tests/integration/api_repo_secrets_test.go b/tests/integration/api_repo_secrets_test.go index a6a59818f7..f012f3cb24 100644 --- a/tests/integration/api_repo_secrets_test.go +++ b/tests/integration/api_repo_secrets_test.go @@ -16,7 +16,6 @@ import ( unit_model "forgejo.org/models/unit" "forgejo.org/models/unittest" user_model "forgejo.org/models/user" - "forgejo.org/modules/keying" api "forgejo.org/modules/structs" repo_service "forgejo.org/services/repository" "forgejo.org/tests" @@ -103,10 +102,22 @@ func TestAPIRepoSecrets(t *testing.T) { } for _, c := range cases { - req := NewRequestWithJSON(t, "PUT", fmt.Sprintf("/api/v1/repos/%s/actions/secrets/%s", repo.FullName(), c.Name), api.CreateOrUpdateSecretOption{ - Data: "data", - }).AddTokenAuth(token) + url := fmt.Sprintf("/api/v1/repos/%s/actions/secrets/%s", repo.FullName(), c.Name) + req := NewRequestWithJSON(t, "PUT", url, api.CreateOrUpdateSecretOption{ + Data: " \r\ndàtä\t \r\n ", + }) + req.AddTokenAuth(token) MakeRequest(t, req, c.ExpectedStatus) + + if c.ExpectedStatus < 300 { + secret := unittest.AssertExistsAndLoadBean(t, &secret_model.Secret{RepoID: repo.ID, Name: strings.ToUpper(c.Name)}) + + assert.Equal(t, strings.ToUpper(c.Name), secret.Name) + + value, err := secret.GetDecryptedData() + require.NoError(t, err) + assert.Equal(t, " \ndàtä\t \n ", value) + } } }) @@ -114,20 +125,18 @@ func TestAPIRepoSecrets(t *testing.T) { name := "update_repo_secret_and_test_data" url := fmt.Sprintf("/api/v1/repos/%s/actions/secrets/%s", repo.FullName(), name) - req := NewRequestWithJSON(t, "PUT", url, api.CreateOrUpdateSecretOption{ - Data: "initial", - }).AddTokenAuth(token) + req := NewRequestWithJSON(t, "PUT", url, api.CreateOrUpdateSecretOption{Data: "initial"}) + req.AddTokenAuth(token) MakeRequest(t, req, http.StatusCreated) - req = NewRequestWithJSON(t, "PUT", url, api.CreateOrUpdateSecretOption{ - Data: "changed data", - }).AddTokenAuth(token) + req = NewRequestWithJSON(t, "PUT", url, api.CreateOrUpdateSecretOption{Data: " \r\nchànged data\t\r\n "}) + req.AddTokenAuth(token) MakeRequest(t, req, http.StatusNoContent) secret := unittest.AssertExistsAndLoadBean(t, &secret_model.Secret{Name: strings.ToUpper(name)}) - data, err := keying.ActionSecret.Decrypt(secret.Data, keying.ColumnAndID("data", secret.ID)) + data, err := secret.GetDecryptedData() require.NoError(t, err) - assert.Equal(t, "changed data", string(data)) + assert.Equal(t, " \nchànged data\t\n ", data) }) t.Run("Delete", func(t *testing.T) { diff --git a/tests/integration/api_repo_variables_test.go b/tests/integration/api_repo_variables_test.go index 0cc79bc423..9752ef24e2 100644 --- a/tests/integration/api_repo_variables_test.go +++ b/tests/integration/api_repo_variables_test.go @@ -78,19 +78,22 @@ func TestAPIRepoVariablesTestCreateRepositoryVariable(t *testing.T) { } for _, c := range cases { - req := NewRequestWithJSON(t, "POST", fmt.Sprintf("/api/v1/repos/%s/actions/variables/%s", repo.FullName(), c.Name), api.CreateVariableOption{ - Value: "value" + c.Name, - }).AddTokenAuth(token) + url := fmt.Sprintf("/api/v1/repos/%s/actions/variables/%s", repo.FullName(), c.Name) + + req := NewRequestWithJSON(t, "POST", url, api.CreateVariableOption{Value: " \tvalüé\r\n" + c.Name + " \r\n"}) + req.AddTokenAuth(token) MakeRequest(t, req, c.ExpectedStatus) if c.ExpectedStatus < 300 { - req = NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/actions/variables/%s", repo.FullName(), c.Name)). - AddTokenAuth(token) + req = NewRequest(t, "GET", url) + req.AddTokenAuth(token) res := MakeRequest(t, req, http.StatusOK) + variable := api.ActionVariable{} DecodeJSON(t, res, &variable) + assert.Equal(t, variable.Name, c.Name) - assert.Equal(t, variable.Data, "value"+c.Name) + assert.Equal(t, variable.Data, " \tvalüé\n"+c.Name+" \n") } } } @@ -105,62 +108,92 @@ func TestAPIRepoVariablesUpdateRepositoryVariable(t *testing.T) { variableName := "test_update_var" url := fmt.Sprintf("/api/v1/repos/%s/actions/variables/%s", repo.FullName(), variableName) - req := NewRequestWithJSON(t, "POST", url, api.CreateVariableOption{ - Value: "initial_val", - }).AddTokenAuth(token) + req := NewRequestWithJSON(t, "POST", url, api.CreateVariableOption{Value: "initial_val"}) + req.AddTokenAuth(token) MakeRequest(t, req, http.StatusNoContent) - cases := []struct { - Name string - UpdateName string - ExpectedStatus int - }{ - { - Name: "not_found_var", - ExpectedStatus: http.StatusNotFound, - }, - { - Name: variableName, - UpdateName: "1invalid", - ExpectedStatus: http.StatusBadRequest, - }, - { - Name: variableName, - UpdateName: "invalid@name", - ExpectedStatus: http.StatusBadRequest, - }, - { - Name: variableName, - UpdateName: "ci", - ExpectedStatus: http.StatusBadRequest, - }, - { - Name: variableName, - UpdateName: "forgejo_foo", - ExpectedStatus: http.StatusBadRequest, - }, - { - Name: variableName, - UpdateName: "updated_var_name", - ExpectedStatus: http.StatusNoContent, - }, - { - Name: variableName, - ExpectedStatus: http.StatusNotFound, - }, - { - Name: "updated_var_name", - ExpectedStatus: http.StatusNoContent, - }, - } + t.Run("Accepts only valid variable names", func(t *testing.T) { + cases := []struct { + Name string + UpdateName string + ExpectedStatus int + }{ + { + Name: "not_found_var", + ExpectedStatus: http.StatusNotFound, + }, + { + Name: variableName, + UpdateName: "1invalid", + ExpectedStatus: http.StatusBadRequest, + }, + { + Name: variableName, + UpdateName: "invalid@name", + ExpectedStatus: http.StatusBadRequest, + }, + { + Name: variableName, + UpdateName: "ci", + ExpectedStatus: http.StatusBadRequest, + }, + { + Name: variableName, + UpdateName: "forgejo_foo", + ExpectedStatus: http.StatusBadRequest, + }, + { + Name: variableName, + UpdateName: "updated_var_name", + ExpectedStatus: http.StatusNoContent, + }, + { + Name: variableName, + ExpectedStatus: http.StatusNotFound, + }, + { + Name: "updated_var_name", + ExpectedStatus: http.StatusNoContent, + }, + } - for _, c := range cases { - req := NewRequestWithJSON(t, "PUT", fmt.Sprintf("/api/v1/repos/%s/actions/variables/%s", repo.FullName(), c.Name), api.UpdateVariableOption{ - Name: c.UpdateName, - Value: "updated_val", - }).AddTokenAuth(token) - MakeRequest(t, req, c.ExpectedStatus) - } + for _, c := range cases { + url := fmt.Sprintf("/api/v1/repos/%s/actions/variables/%s", repo.FullName(), c.Name) + requestData := api.UpdateVariableOption{ + Name: c.UpdateName, + Value: "updated_val", + } + req := NewRequestWithJSON(t, "PUT", url, requestData) + req.AddTokenAuth(token) + MakeRequest(t, req, c.ExpectedStatus) + } + }) + + t.Run("Retains special characters", func(t *testing.T) { + variableName := "special_characters" + url := fmt.Sprintf("/api/v1/repos/%s/actions/variables/%s", repo.FullName(), variableName) + + req := NewRequestWithJSON(t, "POST", url, api.CreateVariableOption{Value: "initial_value"}) + req.AddTokenAuth(token) + MakeRequest(t, req, http.StatusNoContent) + + requestData := api.UpdateVariableOption{ + Value: "\r\n \tüpdåtéd\r\n \r\n", + } + req = NewRequestWithJSON(t, "PUT", url, requestData) + req.AddTokenAuth(token) + MakeRequest(t, req, http.StatusNoContent) + + req = NewRequest(t, "GET", url) + req.AddTokenAuth(token) + res := MakeRequest(t, req, http.StatusOK) + + variable := api.ActionVariable{} + DecodeJSON(t, res, &variable) + + assert.Equal(t, "SPECIAL_CHARACTERS", variable.Name) + assert.Equal(t, "\n \tüpdåtéd\n \n", variable.Data) + }) } func TestAPIRepoVariablesDeleteRepositoryVariable(t *testing.T) { diff --git a/tests/integration/api_user_secrets_test.go b/tests/integration/api_user_secrets_test.go index 5b26aa57b1..b32cf5fb19 100644 --- a/tests/integration/api_user_secrets_test.go +++ b/tests/integration/api_user_secrets_test.go @@ -13,7 +13,6 @@ import ( secret_model "forgejo.org/models/secret" "forgejo.org/models/unittest" user_model "forgejo.org/models/user" - "forgejo.org/modules/keying" api "forgejo.org/modules/structs" "forgejo.org/tests" @@ -72,10 +71,17 @@ func TestAPIUserSecrets(t *testing.T) { } for _, c := range cases { - req := NewRequestWithJSON(t, "PUT", fmt.Sprintf("/api/v1/user/actions/secrets/%s", c.Name), api.CreateOrUpdateSecretOption{ - Data: "data", - }).AddTokenAuth(token) + url := fmt.Sprintf("/api/v1/user/actions/secrets/%s", c.Name) + req := NewRequestWithJSON(t, "PUT", url, api.CreateOrUpdateSecretOption{Data: " \r\ndàtä\t "}) + req.AddTokenAuth(token) MakeRequest(t, req, c.ExpectedStatus) + + if c.ExpectedStatus < 300 { + secret := unittest.AssertExistsAndLoadBean(t, &secret_model.Secret{OwnerID: user.ID, Name: strings.ToUpper(c.Name)}) + data, err := secret.GetDecryptedData() + require.NoError(t, err) + assert.Equal(t, " \ndàtä\t ", data) + } } }) @@ -83,20 +89,18 @@ func TestAPIUserSecrets(t *testing.T) { name := "update_user_secret_and_test_data" url := fmt.Sprintf("/api/v1/user/actions/secrets/%s", name) - req := NewRequestWithJSON(t, "PUT", url, api.CreateOrUpdateSecretOption{ - Data: "initial", - }).AddTokenAuth(token) + req := NewRequestWithJSON(t, "PUT", url, api.CreateOrUpdateSecretOption{Data: "initial"}) + req.AddTokenAuth(token) MakeRequest(t, req, http.StatusCreated) - req = NewRequestWithJSON(t, "PUT", url, api.CreateOrUpdateSecretOption{ - Data: "changed data", - }).AddTokenAuth(token) + req = NewRequestWithJSON(t, "PUT", url, api.CreateOrUpdateSecretOption{Data: " \r\nchängéd\t "}) + req.AddTokenAuth(token) MakeRequest(t, req, http.StatusNoContent) secret := unittest.AssertExistsAndLoadBean(t, &secret_model.Secret{Name: strings.ToUpper(name)}) - data, err := keying.ActionSecret.Decrypt(secret.Data, keying.ColumnAndID("data", secret.ID)) + data, err := secret.GetDecryptedData() require.NoError(t, err) - assert.Equal(t, "changed data", string(data)) + assert.Equal(t, " \nchängéd\t ", data) }) t.Run("Delete", func(t *testing.T) { diff --git a/tests/integration/api_user_variables_test.go b/tests/integration/api_user_variables_test.go index d4806031b5..d37e4324a2 100644 --- a/tests/integration/api_user_variables_test.go +++ b/tests/integration/api_user_variables_test.go @@ -74,19 +74,24 @@ func TestAPIUserVariablesCreateUserVariable(t *testing.T) { } for _, c := range cases { - req := NewRequestWithJSON(t, "POST", fmt.Sprintf("/api/v1/user/actions/variables/%s", c.Name), api.CreateVariableOption{ - Value: "value" + c.Name, - }).AddTokenAuth(token) + url := fmt.Sprintf("/api/v1/user/actions/variables/%s", c.Name) + + req := NewRequestWithJSON(t, "POST", url, api.CreateVariableOption{ + Value: " \tvalüé\r\n" + c.Name + " \r\n", + }) + req.AddTokenAuth(token) MakeRequest(t, req, c.ExpectedStatus) if c.ExpectedStatus < 300 { - req = NewRequest(t, "GET", fmt.Sprintf("/api/v1/user/actions/variables/%s", c.Name)). - AddTokenAuth(token) + req = NewRequest(t, "GET", url) + req.AddTokenAuth(token) res := MakeRequest(t, req, http.StatusOK) + variable := api.ActionVariable{} DecodeJSON(t, res, &variable) + assert.Equal(t, variable.Name, c.Name) - assert.Equal(t, variable.Data, "value"+c.Name) + assert.Equal(t, variable.Data, " \tvalüé\n"+c.Name+" \n") } } } @@ -106,57 +111,87 @@ func TestAPIUserVariablesUpdateUserVariable(t *testing.T) { }).AddTokenAuth(token) MakeRequest(t, req, http.StatusNoContent) - cases := []struct { - Name string - UpdateName string - ExpectedStatus int - }{ - { - Name: "not_found_var", - ExpectedStatus: http.StatusNotFound, - }, - { - Name: variableName, - UpdateName: "1invalid", - ExpectedStatus: http.StatusBadRequest, - }, - { - Name: variableName, - UpdateName: "invalid@name", - ExpectedStatus: http.StatusBadRequest, - }, - { - Name: variableName, - UpdateName: "ci", - ExpectedStatus: http.StatusBadRequest, - }, - { - Name: variableName, - UpdateName: "forgejo_foo", - ExpectedStatus: http.StatusBadRequest, - }, - { - Name: variableName, - UpdateName: "updated_var_name", - ExpectedStatus: http.StatusNoContent, - }, - { - Name: variableName, - ExpectedStatus: http.StatusNotFound, - }, - { - Name: "updated_var_name", - ExpectedStatus: http.StatusNoContent, - }, - } + t.Run("Accepts only valid names", func(t *testing.T) { + cases := []struct { + Name string + UpdateName string + ExpectedStatus int + }{ + { + Name: "not_found_var", + ExpectedStatus: http.StatusNotFound, + }, + { + Name: variableName, + UpdateName: "1invalid", + ExpectedStatus: http.StatusBadRequest, + }, + { + Name: variableName, + UpdateName: "invalid@name", + ExpectedStatus: http.StatusBadRequest, + }, + { + Name: variableName, + UpdateName: "ci", + ExpectedStatus: http.StatusBadRequest, + }, + { + Name: variableName, + UpdateName: "forgejo_foo", + ExpectedStatus: http.StatusBadRequest, + }, + { + Name: variableName, + UpdateName: "updated_var_name", + ExpectedStatus: http.StatusNoContent, + }, + { + Name: variableName, + ExpectedStatus: http.StatusNotFound, + }, + { + Name: "updated_var_name", + ExpectedStatus: http.StatusNoContent, + }, + } - for _, c := range cases { - req := NewRequestWithJSON(t, "PUT", fmt.Sprintf("/api/v1/user/actions/variables/%s", c.Name), api.UpdateVariableOption{ - Name: c.UpdateName, - Value: "updated_val", - }).AddTokenAuth(token) - MakeRequest(t, req, c.ExpectedStatus) - } + for _, c := range cases { + url := fmt.Sprintf("/api/v1/user/actions/variables/%s", c.Name) + req := NewRequestWithJSON(t, "PUT", url, api.UpdateVariableOption{ + Name: c.UpdateName, + Value: "updated_val", + }) + req.AddTokenAuth(token) + MakeRequest(t, req, c.ExpectedStatus) + } + }) + + t.Run("Retains special characters", func(t *testing.T) { + variableName := "special_characters" + url := fmt.Sprintf("/api/v1/user/actions/variables/%s", variableName) + + req := NewRequestWithJSON(t, "POST", url, api.CreateVariableOption{Value: "initial_value"}) + req.AddTokenAuth(token) + MakeRequest(t, req, http.StatusNoContent) + + requestData := api.UpdateVariableOption{ + Value: "\r\n \tüpdåtéd\r\n \r\n", + } + req = NewRequestWithJSON(t, "PUT", url, requestData) + req.AddTokenAuth(token) + MakeRequest(t, req, http.StatusNoContent) + + req = NewRequest(t, "GET", url) + req.AddTokenAuth(token) + res := MakeRequest(t, req, http.StatusOK) + + variable := api.ActionVariable{} + DecodeJSON(t, res, &variable) + + assert.Equal(t, "SPECIAL_CHARACTERS", variable.Name) + assert.Equal(t, "\n \tüpdåtéd\n \n", variable.Data) + }) } func TestAPIUserVariablesDeleteUserVariable(t *testing.T) {