Security & Logic Audit: 9 issues found (2 CRITICAL, 1 HIGH, 3 MEDIUM, 3 LOW)

[lcanady]

Audit Findings

Manual review of flags.ts and mod_test.ts. This library is the authorization primitive for UrsaMU — bugs here propagate into every permission check in the engine.

---

CRITICAL

• [AuthBypass] flags.ts codes() — unknown flag appends literal "false" to output

  The && short-circuit returns the boolean false when a flag is not found, and .reduce() coerces it to the string "false":

  .map((flag) => this.exists(flag) && this.exists(flag).code)
  .reduce((a, b) => (a += b), "")

  codes("admin unknownflag") returns "Afalse" rather than "A". Any caller that uses the returned code string for permission comparisons (e.g., checking character codes in a display or auth layer) will silently receive a corrupted result. Fix: filter out falsy values before reducing, or use this.exists(flag)?.code ?? "".

• [PrivEsc] flags.ts check() — + level expression grants access based on the entity's highest flag level, not the specific flag

  } else if (flag.endsWith("+")) {
    const baseFlag = flag.slice(0, -1);
    const flagExists = this.exists(baseFlag);
    return flagExists && (this.lvl(list) ?? 0) >= (flagExists?.lvl ?? 0);
  }

  this.lvl(list) returns the highest level among all flags the entity has. So check("wizard", "user+") passes — even though the entity does not have the user flag — because wizard.lvl >= user.lvl. This means possession of any single high-level flag satisfies every lower-tier + expression, regardless of which flags are actually on the entity.

---

HIGH

• [DataLoss] flags.ts _add() — re-registering a flag silently drops add, remove, lock, and data callbacks

  When an existing flag is updated via add(), the replacement object only carries {name, code, lvl}:

  return {
    name: t.name.toLowerCase(),
    code: flag.code,
    lvl: flag.lvl || 0,
  };

  All add, remove, lock, and data fields from the original registration are silently lost. If a security hook was registered on a flag (e.g., remove: (data) => revokeAdminState(data)), re-calling flags.add(...) with an updated code or level removes the hook without any warning.

---

MEDIUM

• [FalsyCoercion] flags.ts lvl() and check() — || 0 coerces explicit lvl: 0 identically to undefined

  return acc < (flag?.lvl || 0) ? flag.lvl : acc;
  // and:
  return flagExists && (this.lvl(list) ?? 0) >= (flagExists?.lvl ?? 0);

  The first line uses || 0, which treats lvl: 0 as falsy and substitutes 0 — same as when lvl is absent. A flag explicitly set to lvl: 0 is indistinguishable from one with no lvl property. Use ?? 0 (nullish coalescing) throughout to only substitute for null / undefined.

• [DataMutation] flags.ts set() — mutates the caller's data object in place

  set() performs delete data[item.slice(1)] and data[flag.name] = flag.data directly on the data argument. Callers likely do not expect their object to be modified; this makes the function unsafe to call with a shared or frozen object. The data parameter should be cloned at the start of the function (const result = { ...data }).

• [BrokenDoc] flags.ts check() — README example with spaces around | does not work

  The README documents:

  flags.check("admin user", "admin | moderator")

  But flagExpr.split(" ") splits this into three tokens: "admin", "|", "moderator". The "|" token matches flag.includes("|") → splits to ["", ""] → both are empty strings → compareFlag("", listArray) → always false. The OR check silently fails. The implementation requires no spaces: "admin|moderator". The test file already uses the correct no-space form; the README example should be corrected (or the parser should trim spaces around |).

---

LOW / INFORMATIONAL

• [Inconsistency] flags.ts exists() — code lookup is case-sensitive, name lookup is case-insensitive

  flag.name.toLowerCase() === t.toLowerCase() || flag.code === t

  exists("admin") and exists("ADMIN") both find {name: "admin", code: "A"} by the name branch. But exists("a") and exists("A") behave differently for the code branch: only the exact-case "A" matches. This inconsistency can cause subtle bugs in callers that look up flags by code.

• [PerfDuplication] flags.ts codes() — this.exists(flag) called twice per flag

  .map((flag) => this.exists(flag) && this.exists(flag).code)

  exists() iterates the flags array on each call. The result should be stored in a variable to avoid the double scan.

• [ZeroCoverage] The existing 5 tests cover only happy paths. The following cases are untested:

  • codes() with an unknown flag (returns "false" bug)
  • check() with a + level expression
  • check() with ! negation combined with | OR
  • _add() re-registering a flag with callbacks (callback loss bug)
  • set() with an empty string as the flags argument
  • lvl() with a flag that has an explicit lvl: 0
  • exists() case-sensitivity for code lookup