Address required import review feedback

src/mods/LauncherMods.lua

@@ -460,9 +460,14 @@ function LauncherMods.list(version)
   local ok, result = pcall(function()
     local options = SaveData.loadOptions()
     local manifests = discover()
-    -- A validated copy owned by another installed mod can satisfy the same
-    -- declared MD5 without asking the player to select the ROM twice.
-    local _, importState = RequiredImports.reconcile(manifests)
+    -- Imports are private player grants. Never scan or copy another mod's
+    -- baseroms: a matching public digest is not permission to share the file.
+    local importState = {}
+    for _, manifest in ipairs(manifests) do
+      local rows, missing, missingOptional = RequiredImports.inspect(manifest)
+      importState[manifest.id] = { rows = rows, missing = missing,
+        missingOptional = missingOptional }
+    end
     -- The first build containing game-specific switches turns the old shared
     -- state into one explicit answer per installed mod and game.  Saving here
     -- means users who only visit the launcher still receive the migration.
@@ -968,12 +973,16 @@ function LauncherMods._installZipInner(source, opts)
   end
 
   local dest = "mods/" .. manifest.id
+  local baseromRecovery = "imports/baseroms-recovery/" .. manifest.id
   local existing, installedSomewhere = sameIdTrees(fs, manifest.id)
   if installedSomewhere and not opts.replace then
     cleanup()
     return nil, "a mod named '" .. manifest.id .. "' is already installed"
   end
   local preservedBaseroms = {}
+  -- A previous failed update may have staged the user's files outside mods/ so
+  -- discovery cannot mistake recovery debris for an installed mod.
+  snapshotTree(baseromRecovery, preservedBaseroms)
   if #existing > 0 then
     for _, path in ipairs(existing) do
       snapshotTree(path .. "/baseroms", preservedBaseroms)
@@ -1012,13 +1021,16 @@ function LauncherMods._installZipInner(source, opts)
     end
   end
   if preserveErr then
-    -- Do not report a successful update that discarded user-owned input. Keep
-    -- a best-effort baseroms-only tree for the next retry instead.
+    -- Do not report a successful update that discarded user-owned input, and
+    -- do not leave a manifest-less baseroms tree that resembles an install.
     removeTree(dest)
+    removeTree(baseromRecovery)
     for rel, bytes in pairs(preservedBaseroms) do
-      if bytes ~= nil then CacheFs.write(dest .. "/baseroms/" .. rel, bytes) end
+      if bytes ~= nil then CacheFs.write(baseromRecovery .. "/" .. rel, bytes) end
     end
     copied, copyErr = nil, preserveErr
+  elseif copied then
+    removeTree(baseromRecovery)
   end
   CacheFs.prefix = savedPrefix
   if not copied then

src/mods/Loader.lua

@@ -574,8 +574,8 @@ function Loader:_validate()
           reason = "required import missing: " .. import.name
           break
         end
-        local data = self.fs.read and self.fs.read(path)
-        local valid, importErr = RequiredImports.validateStoredData(import, data)
+        local valid, importErr = RequiredImports.validateStored(
+          manifest, import, self.fs)
         if not valid then
           reason = "required import invalid: " .. import.name
             .. " (" .. tostring(importErr) .. ")"

src/mods/Manifest.lua

@@ -157,6 +157,8 @@ local function parseImports(value, field, required)
     file = SafePath.require(file, field .. " file")
     assert(not file:find("/", 1, true),
       field .. " file must be a filename inside baseroms")
+    assert(file:sub(1, 1) ~= ".",
+      field .. " file must not use a hidden metadata filename")
     assert(not files[file], "duplicate " .. field .. " file: " .. file)
     files[file] = true
 
@@ -181,12 +183,33 @@ local function parseImports(value, field, required)
     local name = entry.name or id
     assert(type(name) == "string" and name ~= "",
       field .. " name must be a non-empty string")
+    local description = entry.description or entry.hint
+    if description ~= nil then
+      assert(type(description) == "string" and description ~= "",
+        field .. " description must be a non-empty string")
+    end
+    local size = entry.size
+    local maxSize = entry.max_size
+    local function validateSize(value, label)
+      if value == nil then return end
+      assert(type(value) == "number" and value > 0 and value % 1 == 0,
+        field .. " " .. label .. " must be a positive integer")
+      assert(value <= 128 * 1024 * 1024,
+        field .. " " .. label .. " exceeds the 128 MiB hard limit")
+    end
+    validateSize(size, "size")
+    validateSize(maxSize, "max_size")
+    assert(not (size and maxSize) or size <= maxSize,
+      field .. " size must not exceed max_size")
     out[#out + 1] = {
       id = id,
       name = scrubUtf8(name),
+      description = scrubUtf8(description),
       file = file,
       md5 = accepted,
       format = format,
+      size = size,
+      max_size = maxSize,
       required = required ~= false,
     }
   end

src/mods/RequiredImports.lua

@@ -23,6 +23,34 @@ local function isRequired(spec)
 end
 
 RequiredImports.specs = allSpecs
+RequiredImports.MAX_BYTES = 128 * 1024 * 1024
+
+local function sizeLabel(bytes)
+  return ("%.1f MiB"):format(bytes / (1024 * 1024))
+end
+
+-- Check size before a caller reads an external or stored file into one large
+-- Lua string. N64 sources may carry a 512-byte copier header, while stored
+-- files are always canonical and therefore must match the declared size.
+function RequiredImports.sizeError(spec, size, stored)
+  if type(size) ~= "number" then return nil end
+  if size > RequiredImports.MAX_BYTES then
+    return ("file is too large (%s; hard limit is %s)")
+      :format(sizeLabel(size), sizeLabel(RequiredImports.MAX_BYTES))
+  end
+  local headerAllowance = not stored and spec and spec.format == "n64" and 512 or 0
+  if spec and spec.size then
+    if size ~= spec.size and size ~= spec.size + headerAllowance then
+      return ("wrong file size (expected %d bytes%s, got %d)")
+        :format(spec.size, headerAllowance > 0 and " or a 512-byte header" or "", size)
+    end
+  end
+  if spec and spec.max_size and size > spec.max_size + headerAllowance then
+    return ("file is too large for this import (maximum %d bytes, got %d)")
+      :format(spec.max_size, size)
+  end
+  return nil
+end
 
 local N64_MAGIC = {
   ["\128\55\18\64"] = "z64", -- big endian / canonical
@@ -44,7 +72,9 @@ function RequiredImports.normalizeN64(data)
     kind = n64KindAt(data, 513)
     if kind then offset = 513 end
   end
-  if not kind then return nil, "not a recognized Nintendo 64 ROM" end
+  if not kind then
+    return nil, "expected an N64 ROM (.z64/.v64/.n64); file signature was not recognized"
+  end
   data = data:sub(offset)
   if kind == "z64" then return data end
 
@@ -89,14 +119,71 @@ function RequiredImports.path(manifest, spec)
 end
 
 local function removedMarker(manifest, spec)
-  return manifest.path .. "/baseroms/." .. spec.id .. ".removed"
+  return manifest.path .. "/baseroms/.required-import-" .. spec.id .. ".removed"
+end
+
+local function receiptPath(manifest, spec)
+  return manifest.path .. "/baseroms/.required-import-" .. spec.id .. ".validated"
+end
+
+RequiredImports.receiptPath = receiptPath
+
+local function parseReceipt(raw)
+  if type(raw) ~= "string" then return nil end
+  local digest, size, modtime = raw:match("^v1\n([%x]+)\n(%d+)\n([^\n]+)\n?$")
+  if not digest then return nil end
+  return digest:lower(), tonumber(size), tonumber(modtime)
+end
+
+local function cachedDigest(manifest, spec, fs, info)
+  -- A size alone cannot detect a same-length replacement. Require modtime as
+  -- well; filesystems that do not expose it simply take the safe hash path.
+  if not (fs and fs.read and info and info.size and info.modtime) then return nil end
+  local digest, size, modtime = parseReceipt(fs.read(receiptPath(manifest, spec)))
+  if digest and size == info.size and modtime == info.modtime
+      and accepts(spec, digest) then
+    return digest
+  end
+  return nil
+end
+
+local function writeReceipt(manifest, spec, digest, info, fs)
+  if not (digest and info and info.size and info.modtime) then return end
+  local path = receiptPath(manifest, spec)
+  local body = ("v1\n%s\n%d\n%s\n")
+    :format(digest, info.size, tostring(info.modtime))
+  if love and fs == love.filesystem then
+    local savedPrefix = CacheFs.prefix
+    CacheFs.prefix = ""
+    CacheFs.write(path, body)
+    CacheFs.prefix = savedPrefix
+  elseif fs and fs.write then
+    fs.write(path, body)
+  end
+end
+
+local function removeReceipt(manifest, spec, fs)
+  local path = receiptPath(manifest, spec)
+  if love and fs == love.filesystem then
+    local savedPrefix = CacheFs.prefix
+    CacheFs.prefix = ""
+    CacheFs.remove(path)
+    CacheFs.prefix = savedPrefix
+  elseif fs and fs.remove then
+    fs.remove(path)
+  end
 end
 
 -- Validate bytes against a declaration.  The returned data is canonicalized
 -- (notably for N64 byte order/header variants) and is what must be stored.
 function RequiredImports.validateData(spec, data, hashFn)
+  local sourceSizeErr = type(data) == "string"
+    and RequiredImports.sizeError(spec, #data, false)
+  if sourceSizeErr then return nil, sourceSizeErr end
   local normalized, normalizeErr = RequiredImports.normalize(spec, data)
   if not normalized then return nil, normalizeErr end
+  local storedSizeErr = RequiredImports.sizeError(spec, #normalized, true)
+  if storedSizeErr then return nil, storedSizeErr end
   local digest, hashErr = hexDigest(normalized, hashFn)
   if not digest then return nil, hashErr end
   if not accepts(spec, digest) then
@@ -105,6 +192,35 @@ function RequiredImports.validateData(spec, data, hashFn)
   return normalized, digest
 end
 
+-- Validate one installed import without reading it when the engine-authored
+-- receipt still matches the file's size and modification time.
+function RequiredImports.validateStored(manifest, spec, fs, hashFn)
+  fs = fs or (love and love.filesystem)
+  if not (fs and fs.getInfo) then return nil, "filesystem is unavailable" end
+  local path = RequiredImports.path(manifest, spec)
+  local info = fs.getInfo(path, "file")
+  if not info then
+    removeReceipt(manifest, spec, fs)
+    return nil, "file is missing"
+  end
+  local sizeErr = RequiredImports.sizeError(spec, info.size, true)
+  if sizeErr then
+    removeReceipt(manifest, spec, fs)
+    return nil, sizeErr
+  end
+  local cached = cachedDigest(manifest, spec, fs, info)
+  if cached then return true, cached, true end
+  removeReceipt(manifest, spec, fs)
+  if not fs.read then return nil, "file could not be read" end
+  local data = fs.read(path)
+  local normalized, detail = RequiredImports.validateStoredData(spec, data, hashFn)
+  if not normalized then return nil, detail end
+  info = fs.getInfo(path, "file") or info
+  info.size = info.size or #data
+  writeReceipt(manifest, spec, detail, info, fs)
+  return true, detail, false
+end
+
 function RequiredImports.validateStoredData(spec, data, hashFn)
   local normalized, detail = RequiredImports.validateData(spec, data, hashFn)
   if not normalized then return nil, detail end
@@ -121,15 +237,12 @@ function RequiredImports.inspect(manifest, fs, hashFn)
     local path = RequiredImports.path(manifest, spec)
     local suppressed = fs and fs.getInfo
       and fs.getInfo(removedMarker(manifest, spec), "file") ~= nil
-    local data = fs and fs.read and fs.read(path) or nil
-    local normalized, detail
-    if data then
-      normalized, detail = RequiredImports.validateStoredData(spec, data, hashFn)
-    end
+    local exists = fs and fs.getInfo and fs.getInfo(path, "file") ~= nil
+    local valid, detail = RequiredImports.validateStored(manifest, spec, fs, hashFn)
     local row = { id = spec.id, name = spec.name, file = spec.file,
-      format = spec.format, path = path, present = normalized ~= nil,
-      digest = normalized and detail or nil,
-      error = data and not normalized and detail or nil,
+      description = spec.description, format = spec.format, path = path,
+      present = valid == true, digest = valid and detail or nil,
+      error = exists and not valid and detail or nil,
       suppressed = suppressed, required = isRequired(spec), spec = spec }
     if not row.present then
       if row.required then missing = missing + 1
@@ -151,8 +264,13 @@ function RequiredImports.importData(manifest, importId, data, opts)
   if not normalized then return nil, digest end
   local savedPrefix = CacheFs.prefix
   CacheFs.prefix = ""
+  CacheFs.remove(receiptPath(manifest, spec))
   local ok, err = CacheFs.write(RequiredImports.path(manifest, spec), normalized)
   if ok then CacheFs.remove(removedMarker(manifest, spec)) end
+  if ok and love and love.filesystem and love.filesystem.getInfo then
+    local info = love.filesystem.getInfo(RequiredImports.path(manifest, spec), "file")
+    writeReceipt(manifest, spec, digest, info, love.filesystem)
+  end
   CacheFs.prefix = savedPrefix
   if not ok then return nil, "could not copy import: " .. tostring(err) end
   return true, digest
@@ -164,6 +282,7 @@ function RequiredImports.remove(manifest, importId)
       local savedPrefix = CacheFs.prefix
       CacheFs.prefix = ""
       CacheFs.remove(RequiredImports.path(manifest, spec))
+      CacheFs.remove(receiptPath(manifest, spec))
       local marked, markErr = CacheFs.write(removedMarker(manifest, spec), "removed\n")
       CacheFs.prefix = savedPrefix
       if not marked then return nil, "could not remember removal: " .. tostring(markErr) end
@@ -173,94 +292,4 @@ function RequiredImports.remove(manifest, importId)
   return nil, "unknown required import: " .. tostring(importId)
 end
 
--- Fill missing imports from another installed mod when its accepted canonical
--- MD5 overlaps.  The source remains inside the engine-owned mods tree, and a
--- fresh validation is performed before every copy.
-function RequiredImports.reconcile(manifests, fs, hashFn)
-  fs = fs or (love and love.filesystem)
-  if not (fs and fs.read) then return {}, {} end
-  local available, state, declaredPaths = {}, {}, {}
-  for _, manifest in ipairs(manifests or {}) do
-    local rows, missing, missingOptional = {}, 0, 0
-    for _, spec in ipairs(allSpecs(manifest)) do
-      local path = RequiredImports.path(manifest, spec)
-      declaredPaths[path] = true
-      local data = fs.read(path)
-      local suppressed = fs.getInfo
-        and fs.getInfo(removedMarker(manifest, spec), "file") ~= nil
-      local normalized, detail
-      if data then
-        normalized, detail = RequiredImports.validateStoredData(spec, data, hashFn)
-        if normalized then available[detail] = normalized end
-      end
-      local row = { id = spec.id, name = spec.name, file = spec.file,
-        format = spec.format, path = path, present = normalized ~= nil,
-        digest = normalized and detail or nil,
-        error = data and not normalized and detail or nil,
-        suppressed = suppressed, required = isRequired(spec), spec = spec }
-      if not row.present then
-        if row.required then missing = missing + 1
-        else missingOptional = missingOptional + 1 end
-      end
-      rows[#rows + 1] = row
-    end
-    state[manifest.id] = { rows = rows, missing = missing,
-      missingOptional = missingOptional }
-  end
-
-
-  -- Compatibility with mods that already maintained their own baseroms
-  -- folder before this manifest field existed: index every other file in an
-  -- installed mod's folder by both raw and (when recognizable) canonical N64
-  -- MD5. Nothing outside the engine-owned mods tree is searched.
-  if fs.getDirectoryItems and fs.getInfo then
-    for _, manifest in ipairs(manifests or {}) do
-      local dir = manifest.path .. "/baseroms"
-      if fs.getInfo(dir, "directory") then
-        for _, name in ipairs(fs.getDirectoryItems(dir) or {}) do
-          local path = dir .. "/" .. name
-          if name:sub(1, 1) ~= "." and not declaredPaths[path]
-              and fs.getInfo(path, "file") then
-            local data = fs.read(path)
-            if data then
-              local rawDigest = hexDigest(data, hashFn)
-              if rawDigest then available[rawDigest] = data end
-              local canonical = RequiredImports.normalizeN64(data)
-              if canonical then
-                local canonicalDigest = hexDigest(canonical, hashFn)
-                if canonicalDigest then available[canonicalDigest] = canonical end
-              end
-            end
-          end
-        end
-      end
-    end
-  end
-
-  local copied = {}
-  for _, manifest in ipairs(manifests or {}) do
-    local entry = state[manifest.id]
-    for _, row in ipairs(entry.rows) do
-      if not row.present and not row.suppressed then
-        for _, digest in ipairs(row.spec.md5) do
-          local data = available[digest]
-          if data then
-            local ok = RequiredImports.importData(manifest, row.id, data,
-              { hash = hashFn })
-            if ok then
-              copied[#copied + 1] = { mod = manifest.id, import = row.id,
-                digest = digest }
-              row.present, row.digest, row.error = true, digest, nil
-              if row.required then entry.missing = entry.missing - 1
-              else entry.missingOptional = entry.missingOptional - 1 end
-            end
-            break
-          end
-        end
-      end
-    end
-  end
-  return copied, state
-end
-
 return RequiredImports