Merge commit from fork

Author: joecorall

main.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	htemplate "html/template"
 	"log/slog"
 	"math/rand"
 	"net"
@@ -15,7 +16,6 @@ import (
 	"slices"
 	"strings"
 	"sync"
-	"text/template"
 	"time"
 
 	"github.com/libops/captcha-protect/internal/helper"
@@ -93,7 +93,7 @@ type CaptchaProtect struct {
 	googlebotIPs       *helper.GooglebotIPs
 	captchaConfig      CaptchaConfig
 	exemptIps          []*net.IPNet
-	tmpl               *template.Template
+	tmpl               *htemplate.Template
 	ipv4Mask           net.IPMask
 	ipv6Mask           net.IPMask
 	protectRoutesRegex []*regexp.Regexp
@@ -116,6 +116,14 @@ type captchaResponse struct {
 	Success bool `json:"success"`
 }
 
+type challengeData struct {
+	SiteKey      string
+	FrontendJS   string
+	FrontendKey  string
+	ChallengeURL string
+	Destination  string
+}
+
 func CreateConfig() *Config {
 	return &Config{
 		RateLimit:                 20,
@@ -217,18 +225,18 @@ func NewCaptchaProtect(ctx context.Context, next http.Handler, config *Config, n
 	}
 	config.ParseHttpMethods(log)
 
-	var tmpl *template.Template
+	var tmpl *htemplate.Template
 	if _, err := os.Stat(config.ChallengeTmpl); os.IsNotExist(err) {
 		log.Warn("Unable to find template file. Using default template", "challengeTmpl", config.ChallengeTmpl)
 		ts := helper.GetDefaultTmpl()
-		tmpl, err = template.New("challenge").Parse(ts)
+		tmpl, err = htemplate.New("challenge").Parse(ts)
 		if err != nil {
 			return nil, fmt.Errorf("unable to parse challenge template: %v", err)
 		}
 	} else if err != nil {
 		return nil, fmt.Errorf("error checking for template file %s: %v", config.ChallengeTmpl, err)
 	} else {
-		tmpl, err = template.ParseFiles(config.ChallengeTmpl)
+		tmpl, err = htemplate.ParseFiles(config.ChallengeTmpl)
 		if err != nil {
 			return nil, fmt.Errorf("unable to parse challenge template file %s: %v", config.ChallengeTmpl, err)
 		}
@@ -590,12 +598,12 @@ func (bc *CaptchaProtect) servePojJS(rw http.ResponseWriter) {
 func (bc *CaptchaProtect) serveChallengePage(rw http.ResponseWriter, destination string) {
 	activeConfig := bc.getActiveCaptchaConfig()
 
-	d := map[string]string{
-		"SiteKey":      bc.config.SiteKey,
-		"FrontendJS":   activeConfig.js,
-		"FrontendKey":  activeConfig.key,
-		"ChallengeURL": bc.config.ChallengeURL,
-		"Destination":  destination,
+	d := challengeData{
+		SiteKey:      bc.config.SiteKey,
+		FrontendJS:   activeConfig.js,
+		FrontendKey:  activeConfig.key,
+		ChallengeURL: bc.config.ChallengeURL,
+		Destination:  destination,
 	}
 
 	rw.Header().Set("Content-Type", "text/html; charset=utf-8")
@@ -662,16 +670,8 @@ func (bc *CaptchaProtect) verifyChallengePage(rw http.ResponseWriter, req *http.
 	if success {
 		bc.verifiedCache.Set(ip, true, exp)
 
-		destination := req.FormValue("destination")
-		if destination == "" {
-			destination = "%2F"
-		}
-		u, err := url.QueryUnescape(destination)
-		if err != nil {
-			bc.log.Error("unable to unescape destination", "destination", destination, "err", err)
-			u = "/"
-		}
-		http.Redirect(rw, req, u, http.StatusFound)
+		destination := normalizeDestination(req.FormValue("destination"))
+		http.Redirect(rw, req, destination, http.StatusFound)
 		return http.StatusFound
 	}
 
@@ -680,6 +680,42 @@ func (bc *CaptchaProtect) verifyChallengePage(rw http.ResponseWriter, req *http.
 	return http.StatusForbidden
 }
 
+func normalizeDestination(destination string) string {
+	if destination == "" {
+		return "/"
+	}
+
+	unescaped, err := url.QueryUnescape(destination)
+	if err == nil && unescaped != destination {
+		if sanitized := sanitizeDestination(unescaped); sanitized != "/" || unescaped == "/" {
+			return sanitized
+		}
+	}
+
+	return sanitizeDestination(destination)
+}
+
+func sanitizeDestination(destination string) string {
+	if destination == "" {
+		return "/"
+	}
+
+	u, err := url.Parse(destination)
+	if err != nil {
+		return "/"
+	}
+
+	if u.IsAbs() || u.Host != "" {
+		return "/"
+	}
+
+	if !strings.HasPrefix(u.Path, "/") {
+		return "/"
+	}
+
+	return u.RequestURI()
+}
+
 func (bc *CaptchaProtect) serveStatsPage(rw http.ResponseWriter, ip string) {
 	// only allow excluded IPs from viewing
 	if !helper.IsIpExcluded(ip, bc.exemptIps) {

main_test.go

@@ -1895,3 +1895,53 @@ func TestGooglebotIPCheckLoopInitialFetchError(t *testing.T) {
 		t.Fatal("did not expect googlebot IPs to update when initial fetch fails")
 	}
 }
+
+func TestServeChallengePageEscapesDestination(t *testing.T) {
+	config := CreateConfig()
+	config.SiteKey = "test-site-key"
+	config.SecretKey = "test-secret-key"
+	config.ProtectRoutes = []string{"/"}
+	config.ChallengeStatusCode = http.StatusTooManyRequests
+
+	bc, err := NewCaptchaProtect(context.Background(), nil, config, "test")
+	if err != nil {
+		t.Fatalf("Failed to create CaptchaProtect: %v", err)
+	}
+
+	rr := httptest.NewRecorder()
+	maliciousDestination := `" /><script>alert(1)</script>`
+
+	bc.serveChallengePage(rr, maliciousDestination)
+
+	body := rr.Body.String()
+	if !strings.Contains(body, `value="&#34; /&gt;&lt;script&gt;alert(1)&lt;/script&gt;"`) {
+		t.Fatalf("expected destination to be HTML-escaped in attribute context, body=%q", body)
+	}
+	if strings.Contains(body, `value="" /><script>alert(1)</script>`) {
+		t.Fatalf("expected raw destination not to be injected into HTML, body=%q", body)
+	}
+}
+
+func TestNormalizeDestination(t *testing.T) {
+	tests := []struct {
+		name        string
+		destination string
+		want        string
+	}{
+		{name: "empty", destination: "", want: "/"},
+		{name: "decoded local path", destination: "/home?foo=bar", want: "/home?foo=bar"},
+		{name: "encoded local path", destination: "%2Fhome%3Ffoo%3Dbar", want: "/home?foo=bar"},
+		{name: "absolute url", destination: "https://evil.com/phish", want: "/"},
+		{name: "protocol relative url", destination: "//evil.com/phish", want: "/"},
+		{name: "relative path", destination: "home", want: "/"},
+		{name: "invalid escape", destination: "%ZZ", want: "/"},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := normalizeDestination(tt.destination); got != tt.want {
+				t.Fatalf("normalizeDestination(%q) = %q, want %q", tt.destination, got, tt.want)
+			}
+		})
+	}
+}