From c48fc578ca9dae596c458599c725e72872c2cc27 Mon Sep 17 00:00:00 2001 From: anxiousintrovert <82425472+anxiousintrovert@users.noreply.github.com> Date: Fri, 14 Aug 2026 17:08:23 -0500 Subject: [PATCH] Address required import review feedback --- .gitignore | 1 + docs/modding.md | 28 ++- .../jni/love/src/modules/system/System.cpp | 2 + src/import/LauncherView.lua | 12 +- src/import/RomImporter.lua | 63 ++++- src/mods/LauncherMods.lua | 24 +- src/mods/Loader.lua | 4 +- src/mods/Manifest.lua | 23 ++ src/mods/RequiredImports.lua | 229 ++++++++++-------- tests/launcher_mods_install_zip_test.lua | 33 +++ tests/mod_required_imports_tests.lua | 91 +++++-- tests/rom_importer_android_mod_pick_test.lua | 18 ++ 12 files changed, 384 insertions(+), 144 deletions(-) diff --git a/.gitignore b/.gitignore index 0f509ed2..16d74675 100644 --- a/.gitignore +++ b/.gitignore @@ -84,3 +84,4 @@ mobile/ios/bundle_id.local # destinations, but source checkouts and packaged mods never ship the files. /mods/*/baseroms/ /imports/baseroms/ +/imports/baseroms-recovery/ diff --git a/docs/modding.md b/docs/modding.md index 621430dc..19c88f0b 100644 --- a/docs/modding.md +++ b/docs/modding.md @@ -48,8 +48,10 @@ Every mod contains a root `manifest.json` defining its metadata, supported games { "id": "stadium2", "name": "Pokemon Stadium 2 ROM", + "description": "Pokemon Stadium 2 (USA), any supported N64 byte order", "file": "stadium2.z64", "format": "n64", + "size": 67108864, "md5": ["00000000000000000000000000000000"] } ], @@ -116,21 +118,35 @@ When a mod supports multiple games (`"games": ["gen1", "gen2"]`), a dependency c out of mod archives while giving every platform the same installation flow. Each object requires a stable `id`, a display `name`, a destination `file` (a filename, never a path), and one MD5 digest or an array of accepted MD5 -digests. `format` is either `"raw"` (the default) or `"n64"`. +digests. `format` is either `"raw"` (the default) or `"n64"`. An optional +`description` gives players dump or region guidance in the import panel. +`size` declares the exact canonical byte length; `max_size` declares a smaller +per-import ceiling when an exact size is not appropriate. Every import also +has an engine-enforced 128 MiB ceiling and is rejected before hashing when its +filesystem reports an invalid size. For `"n64"`, the launcher recognizes `.z64`, `.v64`, and `.n64` byte orders, strips a recognized 512-byte copier header, converts the bytes to canonical big-endian `.z64` order, and then checks MD5. The canonical bytes are written -to `mods//baseroms/`. Another installed mod with an overlapping -accepted MD5 automatically supplies a copy, so the player only selects a ROM -once. Mods read the result with their existing scoped `mod:read` API, for +to `mods//baseroms/`. Each selection is a private grant to that +mod: the launcher never scans or copies another mod's imported files merely +because its manifest names the same digest. Mods read the result with their existing scoped `mod:read` API, for example `mod:read("baseroms/stadium2.z64")`; no host path or new filesystem permission is exposed. Missing `required_imports` block the mod before its entry chunk runs; missing `optional_imports` remain visible in the same launcher panel but do not block loading. -MD5 here identifies a known dump; it is not used as a security or authenticity -guarantee. Mod archives must not include anything beneath `baseroms/`. +MD5 here identifies a known dump because ROM databases commonly publish it; +it is not a security or authenticity guarantee. Do not paste the SHA-1 used by +Gen1Recomp's own game-ROM importer into an import's `md5` field. Mod archives +must not include anything beneath `baseroms/`. The engine records a validation +receipt keyed by file size and modification time so launcher refreshes and +later boots do not repeatedly hash an unchanged imported ROM. + +New mobile code should call `love.system.pickFile("required_import")`. The +older iOS-only `"stadium"` picker kind remains temporarily for compatibility. +Android now returns `false` for unknown picker kinds instead of treating them +as game-ROM picks. ## Mods and Gold (Gen 2) diff --git a/mobile/android/love/src/jni/love/src/modules/system/System.cpp b/mobile/android/love/src/jni/love/src/modules/system/System.cpp index e5f84b24..fce90d63 100644 --- a/mobile/android/love/src/jni/love/src/modules/system/System.cpp +++ b/mobile/android/love/src/jni/love/src/modules/system/System.cpp @@ -196,6 +196,8 @@ bool System::pickFile(const char *kind) const dest = "picked_required_import.bin"; else if (strcmp(kind, "rom") == 0) dest = "picked_rom.gb"; + // Unknown kinds used to fall through to the ROM destination. Refuse them + // so a newer Lua caller cannot silently route an unrelated file as a ROM. else return false; } diff --git a/src/import/LauncherView.lua b/src/import/LauncherView.lua index eb4a49d3..a8477d7e 100644 --- a/src/import/LauncherView.lua +++ b/src/import/LauncherView.lua @@ -2877,7 +2877,7 @@ local function buildRequiredImportsModal(imp, m) end local noticeW = w - 2 * pad local noticeH = noticeText and Kit.wrapHeight("small", noticeText, noticeW, 2) or 0 - local rowH = math.max(math.floor(56 * m.s), m.btnH) + local rowH = math.max(math.floor(70 * m.s), m.btnH) local perPage = math.min(4, math.max(1, #imports)) local pagerH = #imports > perPage and math.max(Kit.tapMin(), math.floor(30 * m.s)) or 0 local h = pad + Kit.textHeight("button") + math.floor(4 * m.s) @@ -2914,12 +2914,18 @@ local function buildRequiredImportsModal(imp, m) local textW = actionX - innerX - math.floor(8 * m.s) Kit.text("small", Kit.ellipsize("small", row.name, textW), innerX, cy + math.floor(8 * m.s), PAL.heading) + local stateY = cy + math.floor(8 * m.s) + Kit.textHeight("small") + + math.floor(3 * m.s) + if row.description and row.description ~= "" then + Kit.text("micro", Kit.ellipsize("micro", row.description, textW), + innerX, stateY, PAL.muted) + stateY = stateY + Kit.textHeight("micro") + math.floor(2 * m.s) + end local state = row.present and Strings("Ready - %s", row.file) or (row.error and Strings("Invalid file - choose again") or (row.required and Strings("Required - %s", row.file) or Strings("Optional - %s", row.file))) - Kit.text("micro", Kit.ellipsize("micro", state, textW), innerX, - cy + math.floor(8 * m.s) + Kit.textHeight("small") + math.floor(3 * m.s), + Kit.text("micro", Kit.ellipsize("micro", state, textW), innerX, stateY, row.present and PAL.green or (row.required and PAL.yellow or PAL.muted)) btn(imp, actionX, cy + (rowH - m.btnH) / 2, actionW, m.btnH, "req-pick-" .. mod.id .. "-" .. importId, diff --git a/src/import/RomImporter.lua b/src/import/RomImporter.lua index 3336ff5d..1b178af4 100644 --- a/src/import/RomImporter.lua +++ b/src/import/RomImporter.lua @@ -344,6 +344,14 @@ local function readExternalPath(path) return data end +local function externalFileSize(path) + local file = io.open(path, "rb") + if not file then return nil end + local size = file:seek("end") + file:close() + return size +end + local function readDroppedFile(file) local ok, openError = file:open("r") if not ok then return nil, openError end @@ -1131,11 +1139,11 @@ local function chooseSav() return nil end --- Generic user-supplied dependency picker. Validation is manifest-driven, --- so the dialog intentionally permits every file extension; a wrong choice --- cannot reach the mod because its canonical MD5 must match first. -local function chooseRequiredFile(label) - local prompt = shellSafe("Choose " .. tostring(label or "required file")) +-- Generic user-supplied dependency picker. Keep the native prompt entirely +-- engine-owned: manifest labels are untrusted and must never enter shell +-- command templates. The LÖVE modal already shows the specific import name. +local function chooseRequiredFile() + local prompt = shellSafe(Strings("Choose required mod file")) local platform = love.system.getOS() if platform == "OS X" then return commandOutput( @@ -1819,6 +1827,13 @@ local function requiredManifest(self, modId) return nil end +local function requiredSpec(manifest, importId) + for _, candidate in ipairs(require("src.mods.RequiredImports").specs(manifest)) do + if candidate.id == importId then return candidate end + end + return nil +end + local function requiredImportNotice(self, modId, importId, text) self.requiredImportNotice = { modId = modId, @@ -1850,6 +1865,21 @@ function RomImporter:_importRequiredData(modId, importId, data) end function RomImporter:_importRequiredSource(modId, importId, source) + local manifest = requiredManifest(self, modId) + local spec = manifest and requiredSpec(manifest, importId) + if not spec then + requiredImportNotice(self, modId, importId, "Import declaration was not found.") + self.modNotice = nil + return nil + end + local info = love.filesystem.getInfo(source, "file") + local size = info and info.size or externalFileSize(source) + local sizeErr = require("src.mods.RequiredImports").sizeError(spec, size, false) + if sizeErr then + requiredImportNotice(self, modId, importId, sizeErr) + self.modNotice = nil + return nil + end local data = love.filesystem.read(source) if not data then data = readExternalPath(source) end if not data then @@ -1881,24 +1911,31 @@ function RomImporter:chooseRequiredImport(modId, importId) if self.workState == "working" then return end local manifest = requiredManifest(self, modId) if not manifest then return end - local spec - for _, candidate in ipairs(require("src.mods.RequiredImports").specs(manifest)) do - if candidate.id == importId then spec = candidate break end - end + local spec = requiredSpec(manifest, importId) if not spec then return end if self.isNX then local inbox = "imports/baseroms" love.filesystem.createDirectory(inbox) + local lastError for _, name in ipairs(love.filesystem.getDirectoryItems(inbox) or {}) do if name:sub(1, 1) ~= "." then local path = inbox .. "/" .. name - local data = love.filesystem.read(path) + local info = love.filesystem.getInfo(path, "file") + local sizeErr = info and require("src.mods.RequiredImports") + .sizeError(spec, info.size, false) + local data = not sizeErr and love.filesystem.read(path) or nil if data and self:_importRequiredData(modId, importId, data) then return end + if sizeErr then lastError = sizeErr + elseif self.requiredImportNotice + and self.requiredImportNotice.modId == modId + and self.requiredImportNotice.importId == importId then + lastError = self.requiredImportNotice.text + end end end - requiredImportNotice(self, modId, importId, - "No matching file in imports/baseroms/. Copy it there over MTP, then try again.") + requiredImportNotice(self, modId, importId, lastError + or "No matching file in imports/baseroms/. Copy it there over MTP, then try again.") self.modNotice = nil return end @@ -1925,7 +1962,7 @@ function RomImporter:chooseRequiredImport(modId, importId) return end - local path = chooseRequiredFile(spec.name) + local path = chooseRequiredFile() if path then self:_importRequiredSource(modId, importId, path) end end diff --git a/src/mods/LauncherMods.lua b/src/mods/LauncherMods.lua index 6679d977..87e51acb 100644 --- a/src/mods/LauncherMods.lua +++ b/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 diff --git a/src/mods/Loader.lua b/src/mods/Loader.lua index 6875f38a..1a46dbf9 100644 --- a/src/mods/Loader.lua +++ b/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) .. ")" diff --git a/src/mods/Manifest.lua b/src/mods/Manifest.lua index 78bba2b9..1fef0295 100644 --- a/src/mods/Manifest.lua +++ b/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 diff --git a/src/mods/RequiredImports.lua b/src/mods/RequiredImports.lua index 3904da3f..167c263f 100644 --- a/src/mods/RequiredImports.lua +++ b/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 diff --git a/tests/launcher_mods_install_zip_test.lua b/tests/launcher_mods_install_zip_test.lua index fc73a186..2ef9dd51 100644 --- a/tests/launcher_mods_install_zip_test.lua +++ b/tests/launcher_mods_install_zip_test.lua @@ -17,6 +17,7 @@ local ARCHIVE = { local files, dirs, arch = {}, {}, {} local fileDataMounts, pathMounts, stagedTemps = 0, 0, {} local stagedEver = false +local failWriteOnce local function resetFs() for k in pairs(files) do files[k] = nil end @@ -25,6 +26,7 @@ local function resetFs() fileDataMounts, pathMounts = 0, 0 stagedTemps = {} stagedEver = false + failWriteOnce = nil end local function dirChild(key, name) @@ -45,6 +47,10 @@ end local vfs = {} function vfs.write(name, data) + if failWriteOnce == name then + failWriteOnce = nil + return nil, "simulated write failure" + end files[name] = data if name:match("^mod_import_") then stagedTemps[name] = true @@ -232,6 +238,33 @@ eq(files["mods/" .. MOD_ID .. "/baseroms/stadium2.z64"], "user-owned-rom", check(files["mods/OldFolder/baseroms/stadium2.z64"] == nil, "the shadow mod tree is still removed after preservation") +-- A preservation write failure keeps recovery bytes outside mods/, where +-- discovery cannot mistake a baseroms-only directory for an installed mod. +resetFs() +files["mods/OldFolder/manifest.json"] = + ('{"id":"%s","name":"Old Copy","version":"0.9.0","entry":"main.lua"}') + :format(MOD_ID) +files["mods/OldFolder/main.lua"] = "return function() end\n" +files["mods/OldFolder/baseroms/stadium2.z64"] = "user-owned-rom" +files["imports/mods/preserve-fail.zip"] = "PK\3\4update" +failWriteOnce = "mods/" .. MOD_ID .. "/baseroms/stadium2.z64" +ok, err = LauncherMods.installZip("imports/mods/preserve-fail.zip", + { replace = true, expectId = MOD_ID }) +check(not ok, "preservation failure rejects the update") +check(files["mods/" .. MOD_ID .. "/manifest.json"] == nil, + "preservation failure leaves no manifest-less tree under mods") +eq(files["imports/baseroms-recovery/" .. MOD_ID .. "/stadium2.z64"], + "user-owned-rom", "preservation failure stages recovery outside mods") + +files["imports/mods/preserve-retry.zip"] = "PK\3\4update" +ok, err = LauncherMods.installZip("imports/mods/preserve-retry.zip", + { replace = true, expectId = MOD_ID }) +check(ok == true, "retry restores staged baseroms (" .. tostring(err) .. ")") +eq(files["mods/" .. MOD_ID .. "/baseroms/stadium2.z64"], "user-owned-rom", + "retry restores the recovered baserom into the installed mod") +check(files["imports/baseroms-recovery/" .. MOD_ID .. "/stadium2.z64"] == nil, + "successful retry clears baserom recovery debris") + -- #834: a manifest-less mods/ tree (interrupted copy debris) must not -- block a plain re-import as "already installed" resetFs() diff --git a/tests/mod_required_imports_tests.lua b/tests/mod_required_imports_tests.lua index 2d068508..271465b5 100644 --- a/tests/mod_required_imports_tests.lua +++ b/tests/mod_required_imports_tests.lua @@ -17,13 +17,17 @@ local manifest = Manifest.validate({ id = "stadium_fx", name = "Stadium FX", version = "1.0.0", entry = "main.lua", required_imports = { { id = "stadium2", name = "Stadium 2", file = "stadium2.z64", - format = "n64", md5 = { DIGEST, DIGEST:upper() } }, + description = "USA dump", format = "n64", size = 8, + md5 = { DIGEST, DIGEST:upper() } }, }, }, "mods/stadium_fx") eq(#manifest.required_imports, 1, "required import parses") eq(#manifest.required_imports[1].md5, 1, "accepted MD5 values normalize and dedupe") eq(manifest.required_imports[1].md5[1], DIGEST, "MD5 is lowercase") +eq(manifest.required_imports[1].description, "USA dump", + "import description is preserved for the picker UI") +eq(manifest.required_imports[1].size, 8, "exact import size parses") local optionalManifest = Manifest.validate({ id = "optional_fx", name = "Optional FX", version = "1.0.0", entry = "main.lua", @@ -49,6 +53,15 @@ check(not pcall(Manifest.validate, { id = "bad", name = "Bad", version = "1", entry = "main.lua", required_imports = { { id = "rom", file = "rom.z64", md5 = "short" } }, }), "malformed MD5 is refused") +check(not pcall(Manifest.validate, { + id = "bad", name = "Bad", version = "1", entry = "main.lua", + required_imports = { { id = "rom", file = ".rom.removed", md5 = DIGEST } }, +}), "hidden import filenames cannot collide with engine metadata") +check(not pcall(Manifest.validate, { + id = "bad", name = "Bad", version = "1", entry = "main.lua", + required_imports = { { id = "rom", file = "rom.bin", md5 = DIGEST, + max_size = RequiredImports.MAX_BYTES + 1 } }, +}), "manifest import sizes cannot exceed the hard limit") local canonical = "\128\55\18\64ABCD" local v64 = "\55\128\64\18BADC" @@ -79,17 +92,14 @@ local target = Manifest.validate({ { id = "same_rom", file = "source.z64", format = "n64", md5 = DIGEST }, }, }, "mods/other_fx") -local copied = RequiredImports.reconcile({ manifest, target }, love.filesystem, fakeHash) -eq(#copied, 1, "matching installed import is reused") -eq(love.filesystem.read("mods/other_fx/baseroms/source.z64"), canonical, - "reuse creates a private per-mod copy") +local targetRows, targetMissing = RequiredImports.inspect(target, + love.filesystem, fakeHash) +eq(targetMissing, 1, "matching hashes do not silently share another mod's import") +check(not targetRows[1].present, + "a mod needs its own explicit player-selected file") check(RequiredImports.remove(target, "same_rom"), "a required import can be removed") eq(love.filesystem.read("mods/other_fx/baseroms/source.z64"), nil, "remove deletes this mod's private copy") -local copiedAfterRemove = RequiredImports.reconcile({ manifest, target }, - love.filesystem, fakeHash) -eq(#copiedAfterRemove, 0, - "an explicit removal is not immediately undone by automatic reuse") check(RequiredImports.importData(target, "same_rom", canonical, { hash = fakeHash }), "choosing the file again clears the removal decision") @@ -103,16 +113,69 @@ local legacyTarget = Manifest.validate({ }, }, "mods/legacy_user") love.filesystem.write("mods/legacy/baseroms/manually-imported.v64", v64) -local legacyCopied = RequiredImports.reconcile({ legacy, legacyTarget }, +local legacyRows, legacyMissing = RequiredImports.inspect(legacyTarget, love.filesystem, fakeHash) -eq(#legacyCopied, 1, "an undeclared legacy baserom can satisfy a new declaration") -eq(love.filesystem.read("mods/legacy_user/baseroms/legacy-source.z64"), canonical, - "legacy reuse still stores canonical bytes") +eq(legacyMissing, 1, "undeclared files in another mod are never indexed") +check(not legacyRows[1].present, "legacy baseroms remain private to their mod") + +local capped = Manifest.validate({ + id = "capped", name = "Capped", version = "1.0.0", entry = "main.lua", + required_imports = { + { id = "small", file = "small.bin", md5 = DIGEST, max_size = 4 }, + }, +}, "mods/capped") +local tooLarge, sizeWhy = RequiredImports.validateData( + capped.required_imports[1], "12345", fakeHash) +eq(tooLarge, nil, "per-import size cap rejects before hashing") +check(tostring(sizeWhy):find("too large", 1, true) ~= nil, + "size rejection explains the limit") + +-- A successful validation writes an engine receipt. Matching size + modtime +-- lets later launcher refreshes avoid reading and hashing the ROM again. +local cacheFiles = { + ["mods/cache/baseroms/source.z64"] = canonical, +} +local dataReads = 0 +local cacheModtime = 123 +local cacheFs = { + getInfo = function(path, kind) + local data = cacheFiles[path] + if data then return { type = "file", size = #data, modtime = cacheModtime } end + return nil + end, + read = function(path) + if path == "mods/cache/baseroms/source.z64" then dataReads = dataReads + 1 end + return cacheFiles[path] + end, + write = function(path, data) cacheFiles[path] = data return true end, + remove = function(path) cacheFiles[path] = nil return true end, +} +local cacheManifest = Manifest.validate({ + id = "cache", name = "Cache", version = "1.0.0", entry = "main.lua", + required_imports = { + { id = "source", file = "source.z64", format = "n64", size = 8, + md5 = DIGEST }, + }, +}, "mods/cache") +local cacheRows = RequiredImports.inspect(cacheManifest, cacheFs, fakeHash) +check(cacheRows[1].present, "initial cached import validation succeeds") +cacheRows = RequiredImports.inspect(cacheManifest, cacheFs, function() + error("unchanged cached import should not be hashed again") +end) +check(cacheRows[1].present, "validation receipt satisfies the next refresh") +eq(dataReads, 1, "unchanged imported ROM is read only once") +cacheFiles["mods/cache/baseroms/source.z64"] = "BADBYTES" +cacheModtime = 124 +cacheRows = RequiredImports.inspect(cacheManifest, cacheFs, fakeHash) +check(not cacheRows[1].present, "changed imported ROM bypasses a stale receipt") +eq(dataReads, 2, "changed imported ROM is read again") +eq(cacheFiles[RequiredImports.receiptPath(cacheManifest, cacheManifest.required_imports[1])], + nil, "stale validation receipt is removed") local rejected, why = RequiredImports.importData(target, "same_rom", "wrong", { hash = fakeHash }) eq(rejected, nil, "mismatched selection is rejected") -check(tostring(why):find("Nintendo 64", 1, true) ~= nil, +check(tostring(why):find("N64 ROM (.z64/.v64/.n64)", 1, true) ~= nil, "normalization failure explains the selected format") love.filesystem.write("mods/launcher_needs/manifest.json", ([[{ diff --git a/tests/rom_importer_android_mod_pick_test.lua b/tests/rom_importer_android_mod_pick_test.lua index f863687e..b958e75b 100644 --- a/tests/rom_importer_android_mod_pick_test.lua +++ b/tests/rom_importer_android_mod_pick_test.lua @@ -135,6 +135,24 @@ check(ri.requiredImportNotice.text:find("MD5 mismatch", 1, true) ~= nil, check(ri.modNotice == nil, "required import rejection is not hidden in the general Mods notice") +-- Reported size is checked before the selected file is read into Lua. +local savedGetInfo = love.filesystem.getInfo +love.filesystem.getInfo = function(name, kind) + if name == "oversized_required.bin" then + return { type = "file", size = 10 } + end + return savedGetInfo(name, kind) +end +ri.mods[1].manifest.required_imports[1].max_size = 4 +ri._importRequiredSource = RomImporter._importRequiredSource +ri._importRequiredData = function(self) self._oversizedWasRead = true end +ri:_importRequiredSource("needs_source", "source", "oversized_required.bin") +check(not ri._oversizedWasRead, "oversized required file is rejected before import") +check(ri.requiredImportNotice.text:find("too large", 1, true) ~= nil, + "oversized required file reports its size error in the modal") +ri.mods[1].manifest.required_imports[1].max_size = nil +love.filesystem.getInfo = savedGetInfo + ri.nativePicker = true ri._importRequiredSource = function(self, modId, importId, source) self._requiredImported = { modId = modId, importId = importId, source = source }