fix(uiasset): bound release download to 64 MiB; log cache metadata write failures

aldinokemal · aldinokemal · commit 8b03695c0556 · 2026-07-14T07:31:59.000+07:00
Review follow-ups (kilo, #766): io.ReadAll on the asset body was unbounded, so a hostile or misconfigured release could exhaust memory — the download now reads at most maxAssetBytes+1 and refuses oversized assets explicitly (a bare LimitReader would silently truncate and, absent a release digest, serve a truncated file). meta.json write failures are now logged instead of dropped.
diff --git a/src/infrastructure/uiasset/cache.go b/src/infrastructure/uiasset/cache.go
@@ -7,6 +7,8 @@ import (
 	"path/filepath"
 	"strings"
 	"time"
+
+	"github.com/sirupsen/logrus"
 )
 
 const (
@@ -69,7 +71,9 @@ func (m *Manager) persistMeta(tag, sha, etag string) {
 		DownloadedAt: time.Now().UTC(),
 	}
 	if raw, err := json.MarshalIndent(meta, "", "  "); err == nil {
-		_ = atomicWrite(filepath.Join(m.cfg.CacheDir, metaFileName), raw)
+		if writeErr := atomicWrite(filepath.Join(m.cfg.CacheDir, metaFileName), raw); writeErr != nil {
+			logrus.Warnf("[UI_ASSET] cache metadata write failed (ETag/tag lost on restart): %v", writeErr)
+		}
 	}
 }
 
diff --git a/src/infrastructure/uiasset/github.go b/src/infrastructure/uiasset/github.go
@@ -16,6 +16,11 @@ import (
 // githubAPIBase is a variable so tests can point it at a local server.
 var githubAPIBase = "https://api.github.com"
 
+// maxAssetBytes bounds the release download so a hostile or misconfigured
+// release cannot exhaust memory: the dashboard is a single HTML file (~1 MB),
+// so 64 MiB is generous headroom. A variable so tests can lower it.
+var maxAssetBytes int64 = 64 << 20
+
 type releaseAsset struct {
 	Name               string `json:"name"`
 	Digest             string `json:"digest"` // "sha256:<hex>", may be empty
@@ -131,9 +136,14 @@ func (m *Manager) download(ctx context.Context, url string) ([]byte, string, err
 	}
 
 	hasher := sha256.New()
-	body, err := io.ReadAll(io.TeeReader(resp.Body, hasher))
+	// Read one byte past the limit so truncation is detected as an explicit
+	// error instead of a silently short (and wrongly hashed) asset.
+	body, err := io.ReadAll(io.LimitReader(io.TeeReader(resp.Body, hasher), maxAssetBytes+1))
 	if err != nil {
 		return nil, "", err
 	}
+	if int64(len(body)) > maxAssetBytes {
+		return nil, "", fmt.Errorf("asset exceeds the %d MiB download limit; refusing", maxAssetBytes>>20)
+	}
 	return body, hex.EncodeToString(hasher.Sum(nil)), nil
 }
diff --git a/src/infrastructure/uiasset/manager_test.go b/src/infrastructure/uiasset/manager_test.go
@@ -226,6 +226,24 @@ func TestPinnedSHARejectsTamperedCache(t *testing.T) {
 	assert.False(t, ok)
 }
 
+func TestEnsureLatestRejectsOversizedAsset(t *testing.T) {
+	previous := maxAssetBytes
+	maxAssetBytes = 16
+	t.Cleanup(func() { maxAssetBytes = previous })
+
+	fake := newFakeGithub(t, "v1.0.0", []byte("<html>this asset is larger than the limit</html>"))
+	manager := newTestManager(t, fake)
+
+	err := manager.EnsureLatest(context.Background())
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "download limit")
+
+	_, _, ok := manager.Content()
+	assert.False(t, ok, "oversized asset must not be served")
+	_, statErr := os.Stat(filepath.Join(manager.cfg.CacheDir, "index.html"))
+	assert.True(t, os.IsNotExist(statErr), "oversized asset must not be cached")
+}
+
 func TestFallbackHTMLMentionsRepoAndVersion(t *testing.T) {
 	page := string(FallbackHTML("v9.0.0", "aldinokemal/gowa-ui"))
 	assert.Contains(t, page, "v9.0.0")

Original file line number	Diff line number	Diff line change
`@@ -7,6 +7,8 @@ import (`
`7`	`7`	`"path/filepath"`
`8`	`8`	`"strings"`
`9`	`9`	`"time"`
	`10`	`+`
	`11`	`+ "github.com/sirupsen/logrus"`
`10`	`12`	`)`
`11`	`13`
`12`	`14`	`const (`
`@@ -69,7 +71,9 @@ func (m *Manager) persistMeta(tag, sha, etag string) {`
`69`	`71`	`DownloadedAt: time.Now().UTC(),`
`70`	`72`	`}`
`71`	`73`	`if raw, err := json.MarshalIndent(meta, "", " "); err == nil {`
`72`		`- _ = atomicWrite(filepath.Join(m.cfg.CacheDir, metaFileName), raw)`
	`74`	`+ if writeErr := atomicWrite(filepath.Join(m.cfg.CacheDir, metaFileName), raw); writeErr != nil {`
	`75`	`+ logrus.Warnf("[UI_ASSET] cache metadata write failed (ETag/tag lost on restart): %v", writeErr)`
	`76`	`+ }`
`73`	`77`	`}`
`74`	`78`	`}`
`75`	`79`