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 <limiting-factor@noreply.codeberg.org>

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
 	})
 }
 

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"
 

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,