Merge pull request #12 from StudioLambda/fix/medium-severity-audit-findings

Fix medium-severity audit findings across all modules

Author: ConsoleTVs

contract/hash.go

@@ -3,13 +3,28 @@ package contract
 // Hasher defines the interface for hashing and verifying hashed values.
 // Implementations of Hasher are responsible for generating cryptographic hashes
 // and verifying that values match their corresponding hashes.
+//
+// Both Hash and Check zero the input value slice after use as a security
+// measure. Callers must not reuse the value slice after calling either method.
 type Hasher interface {
 	// Hash computes a cryptographic hash of the given byte slice and returns the hash.
+	// The input value is zeroed after hashing as a security measure.
+	// Callers must not reuse the value slice after calling Hash.
 	// It returns an error if the hashing operation fails.
 	Hash(value []byte) ([]byte, error)
 
 	// Check verifies that the given value matches the provided hash.
+	// The input value is zeroed after verification as a security measure.
+	// Callers must not reuse the value slice after calling Check.
 	// It returns true if the value and hash match, false otherwise.
 	// It returns an error if the verification operation fails.
 	Check(value []byte, hash []byte) (bool, error)
 }
+
+// Rehashable extends [Hasher] with the ability to detect stale hash parameters.
+// Implementations should return true when the given hash was produced with
+// different parameters than the current configuration, indicating the value
+// should be re-hashed on the next successful authentication.
+type Rehashable interface {
+	NeedsRehash(hash []byte) bool
+}

contract/request/query.go

@@ -48,11 +48,13 @@ func HasQuery(r *http.Request, name string) bool {
 //
 // Returns the parameter value if it exists, otherwise the default value.
 func QueryOr(r *http.Request, name string, fallback string) string {
-	if HasQuery(r, name) {
-		return Query(r, name)
+	values := r.URL.Query()
+
+	if !values.Has(name) {
+		return fallback
 	}
 
-	return fallback
+	return values.Get(name)
 }
 
 // QueryInt retrieves a query parameter by name and parses

contract/request/query_test.go

@@ -157,3 +157,33 @@ func TestQueryIntOrReturnsFallbackWhenNotInteger(t *testing.T) {
 
 	require.Equal(t, 1, result)
 }
+
+func TestQueryOrParsesOnceReturnsValueWhenPresent(t *testing.T) {
+	t.Parallel()
+
+	r := httptest.NewRequest(http.MethodGet, "/?key=value", nil)
+
+	result := request.QueryOr(r, "key", "default")
+
+	require.Equal(t, "value", result)
+}
+
+func TestQueryOrParsesOnceReturnsFallbackWhenMissing(t *testing.T) {
+	t.Parallel()
+
+	r := httptest.NewRequest(http.MethodGet, "/", nil)
+
+	result := request.QueryOr(r, "key", "default")
+
+	require.Equal(t, "default", result)
+}
+
+func TestQueryOrParsesOnceReturnsEmptyWhenPresentButEmpty(t *testing.T) {
+	t.Parallel()
+
+	r := httptest.NewRequest(http.MethodGet, "/?key=", nil)
+
+	result := request.QueryOr(r, "key", "default")
+
+	require.Equal(t, "", result)
+}

contract/response/static.go

@@ -1,6 +1,7 @@
 package response
 
 import (
+	"bytes"
 	"encoding/json"
 	"encoding/xml"
 	"errors"
@@ -87,10 +88,18 @@ func String(w http.ResponseWriter, status int, data string) error {
 //   - tmpl: The text template to execute
 //   - data: The data to pass to the template for execution
 func StringTemplate(w http.ResponseWriter, status int, tmpl template.Template, data any) error {
+	var buf bytes.Buffer
+
+	if err := tmpl.Execute(&buf, data); err != nil {
+		return err
+	}
+
 	w.Header().Set("Content-Type", "text/plain; charset=utf-8")
 	w.WriteHeader(status)
 
-	return tmpl.Execute(w, data)
+	_, err := w.Write(buf.Bytes())
+
+	return err
 }
 
 // HTML writes HTML content to the response writer with the
@@ -121,10 +130,18 @@ func HTML(w http.ResponseWriter, status int, data string) error {
 //   - tmpl: The HTML template to execute (must be html/template for XSS safety)
 //   - data: The data to pass to the template for execution
 func HTMLTemplate(w http.ResponseWriter, status int, tmpl htmltemplate.Template, data any) error {
+	var buf bytes.Buffer
+
+	if err := tmpl.Execute(&buf, data); err != nil {
+		return err
+	}
+
 	w.Header().Set("Content-Type", "text/html; charset=utf-8")
 	w.WriteHeader(status)
 
-	return tmpl.Execute(w, data)
+	_, err := w.Write(buf.Bytes())
+
+	return err
 }
 
 // JSON serializes the given data to JSON format and writes it to the

contract/response/static_test.go

@@ -389,3 +389,33 @@ func TestErrUnsafeRedirectMessage(t *testing.T) {
 		response.ErrUnsafeRedirect.Error(),
 	)
 }
+
+func TestStringTemplateBuffersBeforeWritingStatus(t *testing.T) {
+	t.Parallel()
+
+	w := httptest.NewRecorder()
+	tmpl := template.Must(
+		template.New("test").Parse("{{.Name}}"),
+	)
+
+	err := response.StringTemplate(w, http.StatusOK, *tmpl, 42)
+
+	require.Error(t, err)
+	require.Equal(t, http.StatusOK, w.Code)
+	require.Empty(t, w.Body.String())
+}
+
+func TestHTMLTemplateBuffersBeforeWritingStatus(t *testing.T) {
+	t.Parallel()
+
+	w := httptest.NewRecorder()
+	tmpl := htmltemplate.Must(
+		htmltemplate.New("test").Parse("{{.Name}}"),
+	)
+
+	err := response.HTMLTemplate(w, http.StatusOK, *tmpl, 42)
+
+	require.Error(t, err)
+	require.Equal(t, http.StatusOK, w.Code)
+	require.Empty(t, w.Body.String())
+}

framework/cache/memory.go

@@ -2,19 +2,20 @@ package cache
 
 import (
 	"context"
-	"sync"
+	"errors"
 	"time"
 
 	"github.com/patrickmn/go-cache"
 	"github.com/studiolambda/cosmos/contract"
 )
 
-// Memory implements contract.Cache using an in-memory store backed
-// by patrickmn/go-cache. It is suitable for single-process
-// applications and testing scenarios where persistence across
-// restarts is not required.
+// Memory implements [contract.Cache] using an in-memory store backed by
+// patrickmn/go-cache. Note that this dependency is unmaintained since 2017;
+// consider migrating to a maintained alternative for production use.
+//
+// Memory is suitable for single-process applications and testing scenarios
+// where persistence across restarts is not required.
 type Memory struct {
-	mux   sync.Mutex
 	store *cache.Cache
 }
 
@@ -43,6 +44,10 @@ func (memory *Memory) Get(_ context.Context, key string) (any, error) {
 
 // Put stores a value in the in-memory cache with the given TTL.
 // A zero TTL uses the default expiration configured at creation.
+//
+// WARNING: Values are stored by reference. Callers must not mutate
+// values after storing or after retrieval. For safety with mutable
+// types, store copies or use value types.
 func (memory *Memory) Put(_ context.Context, key string, value any, ttl time.Duration) error {
 	memory.store.Set(key, value, ttl)
 
@@ -64,12 +69,8 @@ func (memory *Memory) Has(_ context.Context, key string) (bool, error) {
 	return found, nil
 }
 
-// Pull atomically retrieves and removes the value for the given key.
-// It holds a mutex to prevent races between the get and delete steps.
+// Pull retrieves and removes the value for the given key.
 func (memory *Memory) Pull(ctx context.Context, key string) (any, error) {
-	memory.mux.Lock()
-	defer memory.mux.Unlock()
-
 	val, err := memory.Get(ctx, key)
 
 	if err != nil {
@@ -84,38 +85,40 @@ func (memory *Memory) Pull(ctx context.Context, key string) (any, error) {
 }
 
 // Forever stores a value permanently with no expiration.
+//
+// WARNING: Values are stored by reference. Callers must not mutate
+// values after storing or after retrieval. For safety with mutable
+// types, store copies or use value types.
 func (memory *Memory) Forever(_ context.Context, key string, value any) error {
 	memory.store.Set(key, value, cache.NoExpiration)
 
 	return nil
 }
 
 // Increment atomically increases the integer value stored at key by
-// the given amount. Returns contract.ErrCacheKeyNotFound if the key
+// the given amount. Returns [contract.ErrCacheKeyNotFound] if the key
 // does not exist.
-func (memory *Memory) Increment(ctx context.Context, key string, by int64) (int64, error) {
-	memory.mux.Lock()
-	defer memory.mux.Unlock()
+func (memory *Memory) Increment(_ context.Context, key string, by int64) (int64, error) {
+	result, err := memory.store.IncrementInt64(key, by)
 
-	if found, _ := memory.Has(ctx, key); !found {
+	if err != nil {
 		return 0, contract.ErrCacheKeyNotFound
 	}
 
-	return memory.store.IncrementInt64(key, by)
+	return result, nil
 }
 
 // Decrement atomically decreases the integer value stored at key by
-// the given amount. Returns contract.ErrCacheKeyNotFound if the key
+// the given amount. Returns [contract.ErrCacheKeyNotFound] if the key
 // does not exist.
-func (memory *Memory) Decrement(ctx context.Context, key string, by int64) (int64, error) {
-	memory.mux.Lock()
-	defer memory.mux.Unlock()
+func (memory *Memory) Decrement(_ context.Context, key string, by int64) (int64, error) {
+	result, err := memory.store.DecrementInt64(key, by)
 
-	if found, _ := memory.Has(ctx, key); !found {
+	if err != nil {
 		return 0, contract.ErrCacheKeyNotFound
 	}
 
-	return memory.store.DecrementInt64(key, by)
+	return result, nil
 }
 
 // Remember retrieves the cached value for the given key, or
@@ -129,39 +132,43 @@ func (memory *Memory) Decrement(ctx context.Context, key string, by int64) (int6
 // use golang.org/x/sync/singleflight to deduplicate concurrent
 // calls for the same key.
 func (memory *Memory) Remember(ctx context.Context, key string, ttl time.Duration, compute func() (any, error)) (any, error) {
-	val, err := memory.Get(ctx, key)
+	value, err := memory.Get(ctx, key)
 
 	if err == nil {
-		return val, nil
+		return value, nil
+	}
+
+	if !errors.Is(err, contract.ErrCacheKeyNotFound) {
+		return nil, err
 	}
 
-	val, err = compute()
+	value, err = compute()
 
 	if err != nil {
 		return nil, err
 	}
 
-	_ = memory.Put(ctx, key, val, ttl)
-
-	return val, nil
+	return value, memory.Put(ctx, key, value, ttl)
 }
 
 // RememberForever retrieves the cached value for the given key, or
 // computes and stores it permanently if the key is not found.
 func (memory *Memory) RememberForever(ctx context.Context, key string, compute func() (any, error)) (any, error) {
-	val, err := memory.Get(ctx, key)
+	value, err := memory.Get(ctx, key)
 
 	if err == nil {
-		return val, nil
+		return value, nil
 	}
 
-	val, err = compute()
+	if !errors.Is(err, contract.ErrCacheKeyNotFound) {
+		return nil, err
+	}
+
+	value, err = compute()
 
 	if err != nil {
 		return nil, err
 	}
 
-	_ = memory.Forever(ctx, key, val)
-
-	return val, nil
+	return value, memory.Forever(ctx, key, value)
 }