From ef43d7c615c9e0c7c3f5920da00e56fdf81b9a90 Mon Sep 17 00:00:00 2001
From: Kwonunn <kwonunnx@gmail.com>
Date: Sun, 26 Jan 2025 11:50:03 +0100
Subject: [PATCH] review: fix various issues brought up by Gusted

---
 act/artifactcache/mac.go  |  2 +-
 act/cacheproxy/handler.go | 31 +++++++++++++++++++------------
 2 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/act/artifactcache/mac.go b/act/artifactcache/mac.go
index 0645fa81..d41a76cf 100644
--- a/act/artifactcache/mac.go
+++ b/act/artifactcache/mac.go
@@ -25,7 +25,7 @@ func (h *Handler) validateMac(rundata cacheproxy.RunData) (string, error) {
 	}
 
 	expectedMAC := computeMac(h.secret, rundata.RepositoryFullName, rundata.RunNumber, rundata.Timestamp)
-	if expectedMAC == rundata.RepositoryMAC {
+	if hmac.Equal([]byte(expectedMAC), []byte(rundata.RepositoryMAC)) {
 		return rundata.RepositoryFullName, nil
 	}
 	return rundata.RepositoryFullName, ErrValidation
diff --git a/act/cacheproxy/handler.go b/act/cacheproxy/handler.go
index 42d58d6b..b79fa47f 100644
--- a/act/cacheproxy/handler.go
+++ b/act/cacheproxy/handler.go
@@ -16,6 +16,7 @@ import (
 	"net/http/httputil"
 	"net/url"
 	"regexp"
+	"strconv"
 	"sync"
 	"time"
 
@@ -29,6 +30,10 @@ const (
 	urlBase = "/_apis/artifactcache"
 )
 
+var (
+	urlRegex = regexp.MustCompile(`/(\w+)(/_apis/artifactcache/.+)`)
+)
+
 type Handler struct {
 	router   *httprouter.Router
 	listener net.Listener
@@ -133,8 +138,7 @@ func (h *Handler) newReverseProxy(targetHost string) (*httputil.ReverseProxy, er
 
 	proxy := &httputil.ReverseProxy{
 		Rewrite: func(r *httputil.ProxyRequest) {
-			re := regexp.MustCompile(`/(\w+)(/_apis/artifactcache/.+)`)
-			matches := re.FindStringSubmatch(r.In.URL.Path)
+			matches := urlRegex.FindStringSubmatch(r.In.URL.Path)
 			id := matches[1]
 			data, ok := h.runs.Load(id)
 			var runData = data.(RunData)
@@ -149,12 +153,12 @@ func (h *Handler) newReverseProxy(targetHost string) (*httputil.ReverseProxy, er
 			r.SetURL(targetURL)
 			r.Out.URL.Path = uri
 
-			r.Out.Header.Add("Forgejo-Cache-Repo", runData.RepositoryFullName)
-			r.Out.Header.Add("Forgejo-Cache-RunNumber", runData.RunNumber)
-			r.Out.Header.Add("Forgejo-Cache-RunId", id)
-			r.Out.Header.Add("Forgejo-Cache-Timestamp", runData.Timestamp)
-			r.Out.Header.Add("Forgejo-Cache-MAC", runData.RepositoryMAC)
-			r.Out.Header.Add("Forgejo-Cache-Host", h.ExternalURL())
+			r.Out.Header.Set("Forgejo-Cache-Repo", runData.RepositoryFullName)
+			r.Out.Header.Set("Forgejo-Cache-RunNumber", runData.RunNumber)
+			r.Out.Header.Set("Forgejo-Cache-RunId", id)
+			r.Out.Header.Set("Forgejo-Cache-Timestamp", runData.Timestamp)
+			r.Out.Header.Set("Forgejo-Cache-MAC", runData.RepositoryMAC)
+			r.Out.Header.Set("Forgejo-Cache-Host", h.ExternalURL())
 		},
 	}
 	return proxy, nil
@@ -162,9 +166,7 @@ func (h *Handler) newReverseProxy(targetHost string) (*httputil.ReverseProxy, er
 
 func (h *Handler) ExternalURL() string {
 	// TODO: make the external url configurable if necessary
-	return fmt.Sprintf("http://%s:%d",
-		h.outboundIP,
-		h.listener.Addr().(*net.TCPAddr).Port)
+	return net.JoinHostPort(h.outboundIP, strconv.Itoa(h.listener.Addr().(*net.TCPAddr).Port))
 }
 
 // Informs the proxy of a workflow run that can make cache requests.
@@ -178,7 +180,10 @@ func (h *Handler) AddRun(data RunData) (string, error) {
 	}
 	key := hex.EncodeToString(keyBytes)
 
-	h.runs.Store(key, data)
+	_, loaded := h.runs.LoadOrStore(key, data)
+	if loaded {
+		return "", errors.New("Run id already exists")
+	}
 
 	return key, nil
 }
@@ -219,7 +224,9 @@ func (h *Handler) Close() error {
 func computeMac(secret, repo, run, ts string) string {
 	mac := hmac.New(sha256.New, []byte(secret))
 	mac.Write([]byte(repo))
+	mac.Write([]byte(">"))
 	mac.Write([]byte(run))
+	mac.Write([]byte(">"))
 	mac.Write([]byte(ts))
 	return hex.EncodeToString(mac.Sum(nil))
 }