From 939bb18c66406466715ddadd25dd9ffa6f169e25 Mon Sep 17 00:00:00 2001
From: Ville Vesilehto <ville@vesilehto.fi>
Date: Fri, 21 Mar 2025 17:51:54 +0200
Subject: [PATCH] fix: add proper HTML escaping in renderFormField

Enhances template safety by escaping user-provided values before inserting
them into HTML output. Improves the function by consistently using escaped
variables throughout the implementation.

- Adds template.HTMLEscapeString for all dynamic values
- Updates variable naming for consistency (escapedName, idAttr, etc.)
- Adds tests to verify proper character escaping works as expected

Signed-off-by: Ville Vesilehto <ville@vesilehto.fi>
---
 server/web/templatefunc.go      | 39 ++++++++++++++++++++++--------
 server/web/templatefunc_test.go | 42 +++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+), 10 deletions(-)

diff --git a/server/web/templatefunc.go b/server/web/templatefunc.go
index f7cce06a..ba2cc543 100644
--- a/server/web/templatefunc.go
+++ b/server/web/templatefunc.go
@@ -314,35 +314,53 @@ func RenderForm(obj interface{}) template.HTML {
 // renderFormField returns a string containing HTML of a single form field. In case of select fType, it will retrun
 // select tag with options. Value for select fType must be comma separated string which are use are
 func renderFormField(label, name, fType string, value interface{}, id string, class string, required bool) string {
+	// Format attributes with spaces first
+	idAttr := ""
 	if id != "" {
-		id = " id=\"" + id + "\""
+		idAttr = " id=\"" + template.HTMLEscapeString(id) + "\""
 	}
 
+	classAttr := ""
 	if class != "" {
-		class = " class=\"" + class + "\""
+		classAttr = " class=\"" + template.HTMLEscapeString(class) + "\""
 	}
 
-	requiredString := ""
+	requiredAttr := ""
 	if required {
-		requiredString = " required"
+		requiredAttr = " required"
+	}
+
+	// Escape all string values
+	escapedName := template.HTMLEscapeString(name)
+	escapedLabel := template.HTMLEscapeString(label)
+	escapedType := template.HTMLEscapeString(fType)
+
+	// Handle value specially as it's an interface{}
+	escapedValue := ""
+	if value != nil {
+		escapedValue = template.HTMLEscapeString(fmt.Sprintf("%v", value))
 	}
 
 	if isValidForInput(fType) {
-		return fmt.Sprintf(`%v<input%v%v name="%v" type="%v" value="%v"%v>`, label, id, class, name, fType, value, requiredString)
+		return fmt.Sprintf(`%v<input%v%v name="%v" type="%v" value="%v"%v>`,
+			escapedLabel, idAttr, classAttr, escapedName, escapedType, escapedValue, requiredAttr)
 	}
 
 	if fType == "select" {
-		valueStr, ok := value.(string)
+		rawValueStr, ok := value.(string)
 		if !ok {
 			logs.Error("for select value must comma separated string that are the options for select")
 			return ""
 		}
 
 		var selectBuilder strings.Builder
-		selectBuilder.WriteString(fmt.Sprintf(`%v<select%v%v name="%v"></br>`, label, id, class, name))
+		selectBuilder.WriteString(fmt.Sprintf(`%v<select%v%v name="%v"></br>`,
+			escapedLabel, idAttr, classAttr, escapedName))
 
-		for _, option := range strings.Split(valueStr, ",") {
-			selectBuilder.WriteString(fmt.Sprintf(`  <option value="%v"> %v </option></br>`, option, option))
+		for _, option := range strings.Split(rawValueStr, ",") {
+			escapedOption := template.HTMLEscapeString(option)
+			selectBuilder.WriteString(fmt.Sprintf(`  <option value="%v"> %v </option></br>`,
+				escapedOption, escapedOption))
 		}
 
 		selectBuilder.WriteString(`</select>`)
@@ -350,7 +368,8 @@ func renderFormField(label, name, fType string, value interface{}, id string, cl
 		return selectBuilder.String()
 	}
 
-	return fmt.Sprintf(`%v<%v%v%v name="%v"%v>%v</%v>`, label, fType, id, class, name, requiredString, value, fType)
+	return fmt.Sprintf(`%v<%v%v%v name="%v"%v>%v</%v>`,
+		escapedLabel, escapedType, idAttr, classAttr, escapedName, requiredAttr, escapedValue, escapedType)
 }
 
 // isValidForInput checks if fType is a valid value for the `type` property of an HTML input element.
diff --git a/server/web/templatefunc_test.go b/server/web/templatefunc_test.go
index 24116378..766c45d5 100644
--- a/server/web/templatefunc_test.go
+++ b/server/web/templatefunc_test.go
@@ -18,6 +18,7 @@ import (
 	"html/template"
 	"net/url"
 	"reflect"
+	"strings"
 	"testing"
 	"time"
 )
@@ -583,3 +584,44 @@ func Test_lt(t *testing.T) {
 		}
 	}
 }
+
+func TestRenderFormSecurity(t *testing.T) {
+	type UserProfile struct {
+		DisplayName string `form:"displayName,text,Name:"`
+		Bio         string `form:",textarea"`
+	}
+
+	// Test case 1: Test proper escaping of special characters in attributes
+	specialCharsProfile := UserProfile{
+		DisplayName: `Special " ' < > & Characters`,
+		Bio:         "Normal text content",
+	}
+
+	output := string(RenderForm(&specialCharsProfile))
+
+	// Verify the output has all special characters properly escaped
+	if strings.Contains(output, `"Special "`) {
+		t.Errorf("Quotation mark not properly escaped in attribute")
+	}
+
+	if !strings.Contains(output, `value="Special`) {
+		t.Errorf("Expected escaped attribute value not found")
+	}
+
+	// Test case 2: Test proper escaping of HTML-like content
+	htmlContentProfile := UserProfile{
+		DisplayName: "Normal Name",
+		Bio:         `<div>Sample HTML content</div>`,
+	}
+
+	output = string(RenderForm(&htmlContentProfile))
+
+	// Verify the output has HTML content properly escaped
+	if strings.Contains(output, `<div>`) {
+		t.Errorf("HTML tags not properly escaped in content")
+	}
+
+	if !strings.Contains(output, `&lt;div&gt;`) {
+		t.Errorf("Expected escaped HTML not found")
+	}
+}