Skip to content

Sanitize ModDB release entries at the parser boundary - #375

Merged
Zaldaryon merged 1 commit into
devfrom
fix/moddb-release-shape
Sep 5, 2026
Merged

Sanitize ModDB release entries at the parser boundary#375
Zaldaryon merged 1 commit into
devfrom
fix/moddb-release-shape

Conversation

@Pixnop

@Pixnop Pixnop commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Part of #370, unblocks #374. #371 stopped the crash inside the installed filters; Zaldaryon's review of the beta.8 promotion found that the filters were not the only reader:

useGetCompleteInstalledMods.ts:71-77 still iterates the raw dmod.releases array and reads release.modversion and release.tags first. parseModDetailResponse checks that releases is an array, but it does not validate its members. A response containing { releases: [null] } therefore throws at release.modversion; a release whose tags is null reaches evaluateModCompatibility and throws there. Please sanitize release entries at the ModDB parser boundary or guard this hook and every other raw release consumer. Add a Manage Mods or hook regression that feeds the raw malformed JSON and verifies that loading completes without an exception.

Correct on both counts, and the fix belongs at the boundary.

Why the boundary and not the hook

readModDetail already decides what a detail is: it rejects a bad modid, a blank name, a releases field that is not an array. It just stopped one level short, at the array itself, and let whatever was inside it through untouched. Seven call sites downstream then read those entries as objects with a string modversion and a tags array of strings, because that is the shape the ModDB documents.

Guarding the hook fixes the one path the issue named and leaves the other six holding the same assumption, including the ones nobody has written yet. Cleaning the entries once, where the JSON stops being untrusted text, is a smaller diff and it is the only version that stays true as call sites are added. It also matches what this module already does one function over: readModSummary has cleaned modidstrs and tags with cleanStrings since it was written.

No new dependency, no schema library: there is no zod validation on this endpoint to extend (#314's zod/mini schema covers the versions catalog, which is a different reader), and the checks here are three lines in the style of the rest of the file.

What the parser now guarantees

For every parseModDetailResponse result:

  • tags on the mod is a list of strings. A null, a number, a missing field or a tags that is not a list at all reads as [].
  • releases holds objects only. An entry that is not an object, null included, is dropped.
  • tags on each release gets the same treatment as the mod's own.
  • modversion on each release is a string.

modversion is coerced to "" rather than dropping the release, deliberately. newestReleaseFileId reads releases[0] to build the download URL for the newest file, so removing an entry here would quietly change which file "install newest" picks. Every reader already treats a falsy modversion as unusable: the update scan skips it before semver.valid, the bulk updater matches it against a version it derived from a valid release, the modpack planner compares it for an exact match. An empty one costs that release only the version comparisons it could never have taken part in.

The mod detail type now states those two guarantees (tags: string[], releases: Record<string, unknown>[]), which let newestReleaseFileId drop the cast and the isRecord check it used to need.

Consumers audited

Two functions fetch /api/mod/{id}, and both go through parseModDetailResponse: useQueryMod for the renderer, netHandlers.ts:85 for the mod database visibility ping. Nothing bypasses the parser, so the boundary really is one. Every raw reader of .releases or .tags on a detail, from grep -rn "\.releases\|\.tags" src/renderer src/domain:

Reader What it reads raw Safe because
useGetCompleteInstalledMods.ts:71,77 dmod.releases, release.modversion, release.tags the crash Zaldaryon named: entries are objects, tags is a string list, modversion is a string
useBulkUpdateMods.ts:60 _mod?.releases.find(c => c.modversion === ...) _mod is what the hook above stored, straight from the parser
InstallMod.tsx:53 mod.releases.map(r => r.modidstr) entries are objects, so the map cannot throw
ModReleaseList.tsx:87,88,95 releases.map, release.tags into evaluateModCompatibility and .join(", ") tags is always an array now, join had no guard at all
adapters/importModpack.ts:13 mod.releases.map of mainfile, modidstr, modversion, tags same, and it is the only way a detail reaches the planner
domain/mods/importModpack.ts:167,186,198 detail.releases, release.tags, release.modversion via the adapter above
installedFilters.ts:48,81,92,106,120 _mod?.releases, _mod?.tags, release.tags tolerant on its own since #371, now a second line rather than the only one
moddb.ts newestReleaseFileId releases[0].fileid inside the boundary, reading what readRelease produced

The .tags hits in TagsFilter.tsx, InstalledTagsFilter.tsx and InstalledModsFilterBar.tsx are the filter state and an i18n key, not fields off a detail. No per-consumer guard was added anywhere: none of them bypasses the parser.

Tests

The Manage Mods regression feeds the raw JSON through the real parser and the real hook, no sanitised fixture: a null entry in releases, a release with tags: null, a release with tags: ["1.20.0", null], a release with modversion: null, and the Vanilla Variants mod-level tags: ["Cosmetics", "Crafting", "Storage", null], all on one payload. It asserts the page finishes loading and lists the mod, and a second case asserts the update offer still comes out right (v1.2.0: the newer 1.3.0 release had its tags come in as null, so it reads as undeclared, exactly as it would have if the author had ticked nothing).

Six parser unit tests pin the same shapes plus the empty-modversion decision and the pass-through of every other release field.

Mutants, each applied alone against the committed tree and restored after:

Mutant Result
readModDetail drops the per-entry object filter red, 3 tests: both Manage Mods cases blank the page, and the parser case
readRelease drops the release tags cleaning red, 5 tests: both Manage Mods cases, and 3 parser cases
readModDetail drops the mod-level tags cleaning red, 2 parser tests

The third one is worth naming precisely. It goes red on the parser tests only, and the Manage Mods regression stays green, because #371 already taught installedFilters to read around a null category tag. That is the belt-and-braces the review asked for behaving as intended: the boundary now refuses to emit the shape, and the filters would still survive it if it ever got past.

Testing

On head 8226317:

  • npm run typecheck: passed (node, web, tests).
  • npm run lint:ci: 0 errors, 15 pre-existing React Hooks warnings.
  • npm run format:check: passed.
  • npm run test:coverage: 161 files, 2037 passed, 2 skipped; 93.4% statements, 90.21% branches, 92.7% functions, 94.97% lines locally, floors 87 / 85 / 85 / 89. The CI run on this head is the reference figure if the two differ by a hundredth.
  • npm run build:unpack: passed, Electron 44.1.1 Linux.
  • npx vitest run tests/domain/mods/moddb.test.ts: 33 passed.

Related issues

Type

  • Bug fix
  • Feature
  • Performance
  • Refactor or cleanup
  • Tests only
  • Docs or build

Checklist

  • Targets dev, not main.
  • npm run typecheck passes.
  • npm run lint:ci passes.
  • npm run format:check passes.
  • npm run test:coverage passes, coverage at or above the floor in vitest.config.ts.
  • npm run build:unpack passes.

The beta.7 blank Manage Mods page came from a detail payload the parser
waved through: Vanilla Variants carried a null among its tags, and the
first render died reading it. #371 taught the installed filters to read
around that shape, but useGetCompleteInstalledMods walks the raw
releases array before any filter runs, so a `{ releases: [null] }`
response still threw on release.modversion and a release with
`tags: null` still threw inside evaluateModCompatibility.

readModDetail now cleans what it hands out. A release that is not an
object is dropped, its tags become a list of strings only, and the mod's
own tags get the same treatment. Every reader downstream already assumes
exactly that shape, so fixing it once here is smaller than guarding four
call sites and it covers the ones nobody has written yet.

A release whose modversion is not a string is kept with an empty one
rather than dropped, because newestReleaseFileId reads releases[0] to
build the download URL and removing an entry would quietly change which
file "install newest" picks. Every consumer already treats a falsy
modversion as unusable.

Part of #370.

@Zaldaryon Zaldaryon left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved

The parser now closes the #374 crash path at the ModDB boundary. Non-object release entries are dropped, release tags are normalized to string arrays, and non-string versions become an unusable empty version before the update scan, release list, or modpack planner reads them. The raw JSON Manage Mods regression exercises the real parser and hook.

Verification

  • Exact head 82263177a625fbd7eee629d72e2f318d081d7ba6 is current with dev.
  • Required checks typecheck, lint, test, build (ubuntu-latest), and build (windows-latest) pass.
  • Ubuntu and Windows test matrices and SonarCloud pass. macOS is skipped by repository policy.
  • Focused parser and Manage Mods tests pass.
  • No blocking finding or open review thread remains.

@Zaldaryon
Zaldaryon merged commit 5fa0462 into dev Sep 5, 2026
9 checks passed
@Zaldaryon
Zaldaryon deleted the fix/moddb-release-shape branch September 5, 2026 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants