fix: avoid GitHub API rate limits when downloading firmware (#893)

* fix: avoid GitHub API rate limits when downloading firmware Startup now only downloads binaries for the latest 2 releases; the remaining releases are fetched on demand via the "update firmware" admin action. Asset downloads use the browser_download_url (redirects to S3), which does not count against the GitHub API rate limit, falling back to the API asset endpoint with auth for private repositories. * test for actual firmware file before saving.
2026-08-31 06:57:10 +02:00 · 2026-08-24 13:52:06 -07:00
parent 648399a52a
commit 3e903a392c
7 changed files with 195 additions and 34 deletions
@@ -74,7 +74,7 @@ func run(cmd *cobra.Command, args []string) error {
 					slog.Error("Panic during background firmware update", "panic", r)
 				}
 			}()
-			if err := srv.UpdateFirmwareBinaries(); err != nil {
+			if err := srv.UpdateFirmwareBinaries(server.FirmwareReleasesAtStartup); err != nil {
 				slog.Error("Failed to update firmware binaries in background", "error", err)
 			}
 		}()
@@ -18,8 +18,32 @@ const (
 	// MergedAppOffset is the offset where the app binary starts in a merged firmware image.
 	// Merged binaries contain: bootloader (0x0/0x1000) + partition table (0x8000) + app (0x10000).
 	MergedAppOffset = 0x10000
+
+	// espImageMagic is the ESP-IDF firmware image header magic byte.
+	espImageMagic = 0xE9
+	// esp32BootloaderOffset is where the bootloader starts in classic ESP32 merged images.
+	esp32BootloaderOffset = 0x1000
+	// espImageHeaderMinSize is the ESP-IDF image header size.
+	espImageHeaderMinSize = 24
 )

+// LooksLikeFirmware reports whether data appears to be an ESP32 firmware image:
+// an OTA app binary (magic 0xE9 at offset 0) or a merged image with the
+// bootloader at 0x0 or 0x1000.
+func LooksLikeFirmware(data []byte) bool {
+	if hasESPImageHeader(data) {
+		return true
+	}
+	if len(data) > esp32BootloaderOffset && hasESPImageHeader(data[esp32BootloaderOffset:]) {
+		return true
+	}
+	return false
+}
+
+func hasESPImageHeader(data []byte) bool {
+	return len(data) >= espImageHeaderMinSize && data[0] == espImageMagic
+}
+
 func Generate(firmwareDir string, deviceType data.DeviceType, ssid, password, url string, swapColors bool) ([]byte, error) {
 	filename := deviceType.FirmwareFilename(swapColors)
 	path := filepath.Join(firmwareDir, filename)
@@ -0,0 +1,36 @@
+package firmware
+
+import (
+	"bytes"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestLooksLikeFirmware(t *testing.T) {
+	ota := bytes.Repeat([]byte{0xFF}, 64)
+	ota[0] = espImageMagic
+
+	merged := bytes.Repeat([]byte{0xFF}, esp32BootloaderOffset+64)
+	merged[esp32BootloaderOffset] = espImageMagic
+
+	htmlLogin := []byte("<!DOCTYPE html><html><body>Sign in to GitHub</body></html>")
+
+	tests := []struct {
+		name string
+		data []byte
+		want bool
+	}{
+		{name: "ota image", data: ota, want: true},
+		{name: "merged image with bootloader at 0x1000", data: merged, want: true},
+		{name: "html login page", data: htmlLogin, want: false},
+		{name: "empty", data: nil, want: false},
+		{name: "too short", data: []byte{espImageMagic, 0x01}, want: false},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.want, LooksLikeFirmware(tt.data))
+		})
+	}
+}
@@ -24,7 +24,25 @@ import (
 	"gorm.io/gorm"
 )

-func (s *Server) UpdateFirmwareBinaries() error {
+// FirmwareReleasesToList is the number of releases fetched from the GitHub API.
+const FirmwareReleasesToList = 5
+
+// FirmwareReleasesAtStartup is the number of releases downloaded at server start.
+// The rest can be fetched on demand via the "update firmware" admin action.
+const FirmwareReleasesAtStartup = 2
+
+func (s *Server) githubAPIBaseURL() string {
+	if s.githubAPIBase != "" {
+		return strings.TrimRight(s.githubAPIBase, "/")
+	}
+	return "https://api.github.com"
+}
+
+// UpdateFirmwareBinaries downloads firmware binaries for the most recent
+// maxReleases releases. Asset downloads use the public browser download URL,
+// which does not count against the GitHub API rate limit; the API asset URL is
+// only used as a fallback for private repositories.
+func (s *Server) UpdateFirmwareBinaries(maxReleases int) error {
 	firmwareRepo := os.Getenv("FIRMWARE_REPO")
 	if firmwareRepo == "" {
 		firmwareRepo = "https://github.com/tronbyt/firmware-esp32"
@@ -48,7 +66,7 @@ func (s *Server) UpdateFirmwareBinaries() error {
 	}

 	// Fetch last 5 releases
-	url := fmt.Sprintf("https://api.github.com/repos/%s/%s/releases?per_page=5", owner, repo)
+	url := fmt.Sprintf("%s/repos/%s/%s/releases?per_page=5", s.githubAPIBaseURL(), owner, repo)

 	req, err := http.NewRequest(http.MethodGet, url, nil)
 	if err != nil {
@@ -104,8 +122,9 @@ func (s *Server) UpdateFirmwareBinaries() error {
 	var releases []struct {
 		TagName string `json:"tag_name"`
 		Assets  []struct {
-			Name string `json:"name"`
-			URL  string `json:"url"` // API URL
+			Name               string `json:"name"`
+			URL                string `json:"url"`                  // API URL
+			BrowserDownloadURL string `json:"browser_download_url"` // Direct download URL (not rate limited)
 		} `json:"assets"`
 	}
 	if err := json.NewDecoder(resp.Body).Decode(&releases); err != nil {
@@ -156,7 +175,11 @@ func (s *Server) UpdateFirmwareBinaries() error {
 		"waveshare-s3_merged.bin":    "waveshare-s3_merged.bin",
 	}

-	for _, release := range releases {
+	for i, release := range releases {
+		if i >= maxReleases {
+			break
+		}
+
 		versionDir := filepath.Join(releasesDir, release.TagName)
 		if err := os.MkdirAll(versionDir, 0755); err != nil {
 			slog.Error("Failed to create version dir", "version", release.TagName, "error", err)
@@ -178,62 +201,81 @@ func (s *Server) UpdateFirmwareBinaries() error {

 			slog.Info("Downloading firmware asset", "version", release.TagName, "asset", asset.Name)

-			dReq, err := http.NewRequest(http.MethodGet, asset.URL, nil)
-			if err != nil {
-				slog.Error("Failed to create firmware download request", "asset", asset.Name, "error", err)
-				continue
-			}
-			dReq.Header.Set("Accept", "application/octet-stream")
-			if token != "" {
-				dReq.Header.Set("Authorization", "Bearer "+token)
-			}
+			downloadAsset := func(downloadURL string, useAPIEndpoint bool) bool {
+				dReq, err := http.NewRequest(http.MethodGet, downloadURL, nil)
+				if err != nil {
+					slog.Error("Failed to create firmware download request", "asset", asset.Name, "error", err)
+					return false
+				}
+				if useAPIEndpoint {
+					dReq.Header.Set("Accept", "application/octet-stream")
+					if token != "" {
+						dReq.Header.Set("Authorization", "Bearer "+token)
+					}
+				}

-			dResp, err := client.Do(dReq)
-			if err != nil {
-				slog.Error("Failed to download firmware asset", "asset", asset.Name, "error", err)
-				continue
-			}
-
-			// Closure to handle defer properly in loop
-			func() {
+				dResp, err := client.Do(dReq)
+				if err != nil {
+					slog.Error("Failed to download firmware asset", "asset", asset.Name, "error", err)
+					return false
+				}
 				defer func() {
 					if err := dResp.Body.Close(); err != nil {
 						slog.Error("Failed to close response body", "asset", asset.Name, "error", err)
 					}
 				}()
+
 				if dResp.StatusCode != http.StatusOK {
-					if dResp.StatusCode == http.StatusForbidden {
-						slog.Warn("Failed to download firmware asset (rate limit)", "asset", asset.Name, "status", dResp.StatusCode)
-					} else {
-						slog.Error("Failed to download firmware asset (bad status)", "asset", asset.Name, "status", dResp.StatusCode)
-					}
-					return
+					slog.Error("Failed to download firmware asset", "asset", asset.Name, "status", dResp.StatusCode)
+					return false
 				}

 				tempPath := localPath + ".tmp"
 				outFile, err := os.Create(tempPath)
 				if err != nil {
 					slog.Error("Failed to create temp firmware file", "file", tempPath, "error", err)
-					return
+					return false
 				}

 				if _, err := io.Copy(outFile, dResp.Body); err != nil {
 					_ = outFile.Close()
 					_ = os.Remove(tempPath)
 					slog.Error("Failed to write firmware file", "file", localPath, "error", err)
-					return
+					return false
 				}
 				if err := outFile.Close(); err != nil {
 					_ = os.Remove(tempPath)
 					slog.Error("Failed to close temp firmware file", "file", tempPath, "error", err)
-					return
+					return false
+				}
+
+				content, err := os.ReadFile(tempPath)
+				if err != nil {
+					_ = os.Remove(tempPath)
+					slog.Error("Failed to read temp firmware file", "file", tempPath, "error", err)
+					return false
+				}
+				if !firmware.LooksLikeFirmware(content) {
+					_ = os.Remove(tempPath)
+					slog.Error("Downloaded content is not a firmware binary", "asset", asset.Name)
+					return false
 				}

 				if err := os.Rename(tempPath, localPath); err != nil {
 					slog.Error("Failed to rename firmware file", "from", tempPath, "to", localPath, "error", err)
 					_ = os.Remove(tempPath)
+					return false
 				}
-			}()
+				return true
+			}
+
+			// Prefer the browser download URL: it redirects to S3 and does not
+			// count against the GitHub API rate limit. Fall back to the API
+			// asset endpoint for private repositories.
+			if !downloadAsset(asset.BrowserDownloadURL, false) && token != "" {
+				slog.Info("Retrying firmware download via API endpoint", "version", release.TagName, "asset", asset.Name)
+				downloadAsset(asset.URL, true)
+			}
 		}
 	}

@@ -2,6 +2,7 @@ package server

 import (
 	"context"
+	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
@@ -12,6 +13,9 @@ import (

 	"tronbyt-server/internal/data"
 	"tronbyt-server/internal/firmware"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )

 func TestHandleFirmwareGenerateGet(t *testing.T) {
@@ -101,3 +105,55 @@ func TestHandleFirmwareGeneratePost(t *testing.T) {
 		t.Error("Expected firmware binary in response")
 	}
 }
+
+func TestUpdateFirmwareBinariesRejectsHTMLAndRetriesAPI(t *testing.T) {
+	s := newTestServerAPI(t)
+	s.Config.GitHubToken = "test-token"
+	t.Setenv("FIRMWARE_REPO", "")
+
+	firmwareBytes := make([]byte, 64)
+	firmwareBytes[0] = 0xE9
+
+	var browserHits, apiHits int
+	var apiAccept, apiAuth string
+
+	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		switch {
+		case strings.HasPrefix(r.URL.Path, "/repos/tronbyt/firmware-esp32/releases"):
+			base := "http://" + r.Host
+			w.Header().Set("Content-Type", "application/json")
+			if _, err := fmt.Fprintf(w, `[{"tag_name":"v9.9.9","assets":[{"name":"tidbyt-gen1_firmware.bin","url":"%s/api-asset","browser_download_url":"%s/browser"}]}]`, base, base); err != nil {
+				http.Error(w, err.Error(), http.StatusInternalServerError)
+			}
+		case r.URL.Path == "/browser":
+			browserHits++
+			w.Header().Set("Content-Type", "text/html; charset=utf-8")
+			_, _ = w.Write([]byte("<!DOCTYPE html><html><body>Sign in to GitHub</body></html>"))
+		case r.URL.Path == "/api-asset":
+			apiHits++
+			apiAccept = r.Header.Get("Accept")
+			apiAuth = r.Header.Get("Authorization")
+			w.Header().Set("Content-Type", "application/octet-stream")
+			_, _ = w.Write(firmwareBytes)
+		default:
+			http.NotFound(w, r)
+		}
+	}))
+	t.Cleanup(ts.Close)
+	s.githubAPIBase = ts.URL
+
+	require.NoError(t, s.UpdateFirmwareBinaries(1))
+
+	assert.Equal(t, 1, browserHits)
+	assert.Equal(t, 1, apiHits, "API asset endpoint should be retried after HTML login response")
+	assert.Equal(t, "application/octet-stream", apiAccept)
+	assert.Equal(t, "Bearer test-token", apiAuth)
+
+	localPath := filepath.Join(s.DataDir, "firmware", "releases", "v9.9.9", "tidbyt-gen1.bin")
+	got, err := os.ReadFile(localPath)
+	require.NoError(t, err)
+	assert.Equal(t, firmwareBytes, got)
+
+	_, err = os.Stat(localPath + ".tmp")
+	assert.True(t, os.IsNotExist(err), "temporary file should be cleaned up")
+}
@@ -23,7 +23,7 @@ func (s *Server) handleHealth(w http.ResponseWriter, r *http.Request) {
 }

 func (s *Server) handleUpdateFirmware(w http.ResponseWriter, r *http.Request) {
-	err := s.UpdateFirmwareBinaries()
+	err := s.UpdateFirmwareBinaries(FirmwareReleasesToList)
 	if err != nil {
 		slog.Error("Failed to update firmware binaries", "error", err)
 	}
@@ -60,6 +60,9 @@ type Server struct {

 	UpdateAvailable  bool
 	LatestReleaseURL string
+
+	// githubAPIBase, when set, replaces https://api.github.com (used by tests).
+	githubAPIBase string
 }

 // SchemaCacheBypasser forces cache reads for keys with the given prefix to miss