From 78e1d2eacb4c96fcf67490fce0151fdfb01e8031 Mon Sep 17 00:00:00 2001
From: jpirnay <jens@pirnay.com>
Date: Wed, 20 May 2026 19:34:27 +0200
Subject: [PATCH] Review changes

---
 src/FontInstaller.cpp                         | 13 +++++++++
 src/FontInstaller.h                           |  5 ++++
 .../settings/FontDownloadActivity.cpp         | 28 +++++++++++++++++--
 3 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/src/FontInstaller.cpp b/src/FontInstaller.cpp
index fa099d24..0f03ac8d 100644
--- a/src/FontInstaller.cpp
+++ b/src/FontInstaller.cpp
@@ -30,6 +30,19 @@ bool FontInstaller::isValidFamilyName(const char* name) {
   return true;
 }
 
+bool FontInstaller::isValidFontFileName(const char* name) {
+  if (name == nullptr || name[0] == '\0') return false;
+  // Matches the cap used by the web-server manifest parser; the SD-side path buffer is 128
+  // bytes including the family prefix, so 60 leaves plenty of headroom.
+  static constexpr size_t MAX_FONT_FILE_NAME_LEN = 60;
+  const size_t nameLen = strlen(name);
+  if (nameLen > MAX_FONT_FILE_NAME_LEN) return false;
+  if (name[0] == '/') return false;
+  if (strchr(name, '\\') != nullptr) return false;
+  if (strstr(name, "..") != nullptr) return false;
+  return true;
+}
+
 bool FontInstaller::ensureFamilyDir(const char* familyName) {
   if (!isValidFamilyName(familyName)) {
     LOG_ERR("FONT", "Invalid family name: %s", familyName ? familyName : "<null>");
diff --git a/src/FontInstaller.h b/src/FontInstaller.h
index bb33fadd..3ef0190c 100644
--- a/src/FontInstaller.h
+++ b/src/FontInstaller.h
@@ -25,6 +25,11 @@ class FontInstaller {
   /// Validate a family name: alphanumeric + hyphen + underscore only, no path traversal.
   static bool isValidFamilyName(const char* name);
 
+  /// Validate a font file name as it appears in a manifest entry: non-empty, length-bounded,
+  /// no absolute paths, no backslashes, no traversal components. Slashes are allowed for
+  /// "<family>/<file>"-style entries.
+  static bool isValidFontFileName(const char* name);
+
   /// Ensure /.crosspoint/fonts/<family>/ directory exists.
   bool ensureFamilyDir(const char* familyName);
 
diff --git a/src/activities/settings/FontDownloadActivity.cpp b/src/activities/settings/FontDownloadActivity.cpp
index 5498eb95..43973d01 100644
--- a/src/activities/settings/FontDownloadActivity.cpp
+++ b/src/activities/settings/FontDownloadActivity.cpp
@@ -130,7 +130,13 @@ bool FontDownloadActivity::fetchAndParseManifest() {
     family.description = fObj["description"] | "";
     // styles[] in the JSON is intentionally ignored — see ManifestFamily.
 
+    if (!FontInstaller::isValidFamilyName(family.name.c_str())) {
+      LOG_ERR("FONT", "Manifest entry rejected, invalid family name: %s", family.name.c_str());
+      continue;
+    }
+
     family.totalSize = 0;
+    bool fileNamesOk = true;
     for (JsonObject fileObj : fObj["files"].as<JsonArray>()) {
       ManifestFile file;
       file.name = fileObj["name"] | "";
@@ -139,9 +145,15 @@ bool FontDownloadActivity::fetchAndParseManifest() {
         file.crc32 = fileObj["crc32"].as<uint32_t>();
         file.hasCrc32 = true;
       }
+      if (!FontInstaller::isValidFontFileName(file.name.c_str())) {
+        LOG_ERR("FONT", "Manifest entry rejected, invalid file name in %s: %s", family.name.c_str(), file.name.c_str());
+        fileNamesOk = false;
+        break;
+      }
       family.totalSize += file.size;
       family.files.push_back(std::move(file));
     }
+    if (!fileNamesOk) continue;
 
     family.installed = fontInstaller_.isFamilyInstalled(family.name.c_str());
 
@@ -225,7 +237,8 @@ bool readU8(FsFile& f, uint8_t& v) { return f.read(&v, 1) == 1; }
 bool readU32(FsFile& f, uint32_t& v) {
   uint8_t buf[4];
   if (f.read(buf, 4) != 4) return false;
-  v = buf[0] | (buf[1] << 8) | (buf[2] << 16) | (buf[3] << 24);
+  v = static_cast<uint32_t>(buf[0]) | (static_cast<uint32_t>(buf[1]) << 8) | (static_cast<uint32_t>(buf[2]) << 16) |
+      (static_cast<uint32_t>(buf[3]) << 24);
   return true;
 }
 bool readStr(FsFile& f, std::string& s) {
@@ -460,8 +473,17 @@ void FontDownloadActivity::downloadFamily(int familyIdx) {
   downloadingFamilyHasResumable_ = family.hasResumableDownload;
 
   // Restore families_ regardless of success/error/abort outcome, then merge
-  // back the mutations the impl made on the local family copy.
-  if (restoreFamiliesFromSd() && familyIdx >= 0 && familyIdx < static_cast<int>(families_.size())) {
+  // back the mutations the impl made on the local family copy. Without the
+  // restored manifest the activity can't render the family list, so a failed
+  // restore is fatal — drop to ERROR rather than continuing with empty state.
+  if (!restoreFamiliesFromSd()) {
+    RenderLock lock(*this);
+    state_ = ERROR;
+    pendingErrorAction_ = PendingFontAction::Download;
+    errorMessage_ = "Failed to restore manifest";
+    return;
+  }
+  if (familyIdx >= 0 && familyIdx < static_cast<int>(families_.size())) {
     families_[familyIdx].installed = family.installed;
     families_[familyIdx].hasUpdate = family.hasUpdate;
     families_[familyIdx].hasResumableDownload = family.hasResumableDownload;