From 63b630e21e2ee9ce2ff35d3f578da60801e21c3d Mon Sep 17 00:00:00 2001 From: Brandon Rothweiler Date: Thu, 9 Jul 2026 22:11:03 +0200 Subject: [PATCH] fix: only expand recognized variables when generating repo (#12917) I noticed that when generating a repo from a template repo, if a file in the template repo contains an unrecognized variable reference (not part of the set of variables that Forgejo replaces), it attempts to expand it anyway, and ultimately replaces it with just the variable name, stripping off the `$` or `${}`. For example, if a file contains `Authorization: Bearer ${ACCESS_TOKEN}`, in the generated repo this will be changed to `Authorization: Bearer ACCESS_TOKEN` which is unexpected and undesired. I considered whether it would be possible to fix this by simply making the function passed to `os.Expand()` return `${VARIABLE_NAME}` instead of just `VARIABLE_NAME` for unrecognized keys, but this wouldn't work because it's not possible to distinguish between `$VARIABLE_NAME` and `${VARIABLE_NAME}` syntax, so some variable references would get mangled anyway. Instead, I fixed it by adding a simple regex and using that to perform the replacements instead of `os.Expand()`. I added unit tests and updated one of the generated repo integration tests to validate the functionality. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/12917 Reviewed-by: limiting-factor --- services/repository/generate.go | 24 ++++++++++---- services/repository/generate_test.go | 43 +++++++++++++++++++++++++ tests/integration/repo_generate_test.go | 6 ++-- 3 files changed, 64 insertions(+), 9 deletions(-) diff --git a/services/repository/generate.go b/services/repository/generate.go index af4d16677f..e2ee20f14e 100644 --- a/services/repository/generate.go +++ b/services/repository/generate.go @@ -77,14 +77,24 @@ func generateExpansion(src string, templateRepo, generateRepo *repo_model.Reposi } } - return os.Expand(src, func(key string) string { - if expansion, ok := expansionMap[key]; ok { - if sanitizeFileName { - return fileNameSanitize(expansion) - } - return expansion + return expandTemplateVars(src, expansionMap, sanitizeFileName) +} + +var templateVarPattern = regexp.MustCompile(`\$\w+|\${\w+}`) + +// expandTemplateVars substitutes recognized $VAR and ${VAR} references +// but leaves unknown variables alone. +func expandTemplateVars(src string, expansionMap map[string]string, sanitizeFileName bool) string { + return templateVarPattern.ReplaceAllStringFunc(src, func(match string) string { + key := strings.TrimSuffix(strings.TrimPrefix(match[1:], "{"), "}") + expansion, ok := expansionMap[key] + if !ok { + return match } - return key + if sanitizeFileName { + return fileNameSanitize(expansion) + } + return expansion }) } diff --git a/services/repository/generate_test.go b/services/repository/generate_test.go index da937c5be5..7f0e9d51f6 100644 --- a/services/repository/generate_test.go +++ b/services/repository/generate_test.go @@ -66,6 +66,49 @@ func TestFileNameSanitize(t *testing.T) { assert.Equal(t, "目标", fileNameSanitize("目标")) } +func TestExpandTemplateVars(t *testing.T) { + expansionMap := map[string]string{ + "REPO_NAME": "my-repo", + "REPO_NAME_SNAKE": "my_repo", + "REPO_OWNER": "alice", + "REPO_OWNER_NOT_SANE": "../alice/..", + } + + perlBareVariable := "perl -e '$REPO_NAME::suffix=value ; print $REPO_NAME::suffix=value'" + perlBracedVariable := "perl -e '${REPO_NAME::suffix=value} ; print ${REPO_NAME::suffix=value}'" + + tests := []struct { + name string + src string + sanitizeFileName bool + expected string + }{ + {"bare known var", "$REPO_NAME", false, "my-repo"}, + {"braced known var", "${REPO_NAME}", false, "my-repo"}, + {"transformer suffix", "${REPO_NAME_SNAKE}", false, "my_repo"}, + {"mixed", "$REPO_OWNER/${REPO_NAME}", false, "alice/my-repo"}, + {"unknown braced var", "${UNKNOWN_VAR}", false, "${UNKNOWN_VAR}"}, + {"unknown bare var", "$UNKNOWN_VAR", false, "$UNKNOWN_VAR"}, + {"unknown in context", "Authorization: token ${AUTH_TOKEN}", false, "Authorization: token ${AUTH_TOKEN}"}, + {"known prefix of unknown token", "$REPO_NAMEX", false, "$REPO_NAMEX"}, + {"double dollar unknown", "$$UNKNOWN_VAR", false, "$$UNKNOWN_VAR"}, + {"double dollar known", "$$REPO_NAME", false, "$my-repo"}, + {"one letter variable", "cost is $5", false, "cost is $5"}, + {"lone dollar", "lonely $", false, "lonely $"}, + {"sanitize sane filename in known var", "${REPO_OWNER}", true, "alice"}, + {"sanitize not sane filename in known var", "${REPO_OWNER_NOT_SANE}", true, "__alice__"}, + {"bare perl variables may be replaced", perlBareVariable, false, "perl -e 'my-repo::suffix=value ; print my-repo::suffix=value'"}, + {"braced perl variables will not be replaced", perlBracedVariable, false, perlBracedVariable}, + {"multiline", "line0\nline1 $REPO_OWNER \nline2 ${REPO_NAME}\n", false, "line0\nline1 alice \nline2 my-repo\n"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.expected, expandTemplateVars(tt.src, expansionMap, tt.sanitizeFileName)) + }) + } +} + func TestTransformers(t *testing.T) { input := "Foo_Forgejo-BAR" diff --git a/tests/integration/repo_generate_test.go b/tests/integration/repo_generate_test.go index aec2a89719..9d9bbc9a24 100644 --- a/tests/integration/repo_generate_test.go +++ b/tests/integration/repo_generate_test.go @@ -238,10 +238,12 @@ func TestRepoGenerateTemplating(t *testing.T) { onApplicationRun(t, func(t *testing.T, u *url.URL) { input := `# $REPO_NAME This is a Repo By $REPO_OWNER - ThisIsThe${REPO_NAME}InAnInlineWay` + ThisIsThe${REPO_NAME}InAnInlineWay + CI token ${CI_DEPLOY_TOKEN} is left untouched` expected := `# %s This is a Repo By %s - ThisIsThe%sInAnInlineWay` + ThisIsThe%sInAnInlineWay + CI token ${CI_DEPLOY_TOKEN} is left untouched` template := forgery.CreateRepository(t, nil, &forgery.CreateRepositoryOptions{ IsTemplate: true,