v1.4.9: Validation fixes, Profiler fix, Quest link duplicate fix, Ascension fixes

2026-03-26 06:47:54 -05:00
parent 284f0c3794
commit 1fc9727fee
194 changed files with 21008 additions and 743 deletions
@@ -0,0 +1,479 @@
+---
+paths:
+  - "**/*.lua"
+---
+# Lua Security
+
+> This file extends [common/security.md](../common/security.md) with Lua specific content.
+
+## Mandatory Security Checks (Lua)
+
+Before ANY commit of Lua code:
+- [ ] No hardcoded secrets (API keys, passwords, tokens, webhook URLs)
+- [ ] All user/external inputs validated before processing
+- [ ] No `loadstring` / `load` with untrusted input
+- [ ] No `os.execute` / `io.popen` with user-controlled strings
+- [ ] No unintentional global variables (verified by `luacheck` with `std = "none"`)
+- [ ] Error messages don't leak file paths, stack traces, or internal state
+- [ ] SavedVariables don't store sensitive data in plaintext
+- [ ] Plugin sandbox restricts access to dangerous libraries
+
+## Secrets Management
+
+- NEVER hardcode API keys, tokens, or credentials in Lua source files
+- Use environment variables (`os.getenv("API_KEY")`) for CLI/server Lua
+- Use secure SavedVariables with obfuscation for addon credentials (if absolutely necessary)
+- Fail fast if required secrets are missing at startup
+- Keep `.env` files and SavedVariables files in `.gitignore`
+
+```lua
+-- BAD: Hardcoded secret
+local API_KEY = "sk-abc123secretkey"
+
+-- GOOD: Environment variable with early validation
+local function loadApiKey()
+    local key = os.getenv("API_KEY")
+    if not key or key == "" then
+        error("API_KEY environment variable must be set")
+    end
+    return key
+end
+```
+
+## `loadstring` and Dynamic Code Execution
+
+`loadstring` (Lua 5.0/5.1) / `load` (Lua 5.2+) execute arbitrary strings as code. This is the **single most critical attack surface** in Lua.
+
+**Version note**: `loadstring` exists in Lua 5.0 and 5.1. In Lua 5.2+, `loadstring` was removed and its functionality was merged into `load`. In Lua 5.0, `loadlib` (not `loadstring`) is also available for loading C libraries — it was moved to `package.loadlib` in 5.1 and later.
+
+### Never Do This
+
+```lua
+-- CRITICAL RISK: Arbitrary code execution from user input
+local fn = loadstring(userInput)
+if fn then fn() end
+
+-- CRITICAL RISK: Loading from untrusted file path
+local fn = loadfile(userSuppliedPath)
+if fn then fn() end
+
+-- CRITICAL RISK: Dynamic code from network data
+local fn = loadstring(httpResponse.body)
+```
+
+### When It Is Acceptable
+
+- Deserializing data from a **trusted, internal source** (e.g., `AceSerializer` output from your own SavedVariables written by your own addon)
+- Compile-time / build tooling code that never runs in production
+- MUST have a `-- SECURITY: loadstring used here because ...` comment
+
+### Alternatives to loadstring
+
+| Problem | Use Instead |
+|---------|-------------|
+| Dynamic function dispatch | Function lookup tables |
+| Computed field access | `rawget(table, key)` |
+| Data deserialization | `AceSerializer`, JSON parser, custom binary format |
+| Template expansion | `string.format` or `gsub` with controlled patterns |
+
+```lua
+-- BAD: Dynamic dispatch via loadstring
+local fn = loadstring("return " .. actionName .. "()")
+
+-- GOOD: Function dispatch table
+local actions = {
+    attack = function() return doAttack() end,
+    defend = function() return doDefend() end,
+    heal   = function() return doHeal() end,
+}
+local fn = actions[actionName]
+if fn then fn() end
+```
+
+### Bytecode Loading (5.2+ Security Risk)
+
+Starting in Lua 5.2, bytecode verification was removed. Loading untrusted binary data via `load()` or `loadfile()` can execute arbitrary code even without `loadstring`. **Always restrict to text-only mode when loading untrusted input:**
+
+```lua
+-- CRITICAL: restrict to text mode when source is untrusted
+-- The 4th arg 't' = text only, 'b' = binary only, 'bt' = both (default, unsafe)
+-- In 5.2: mode is 3rd arg; in 5.3+: mode is 4th arg (after chunk name)
+local fn, err = load(untrusted_source, "=(untrusted)", nil, "t")
+
+-- Safe alternative: explicit source validation before loading
+if not isKnownTrustedSource(source) then
+    return nil, "Refused to load untrusted source"
+end
+
+-- In Lua 5.1 and earlier, load() does not accept a mode parameter.
+-- loadstring() only accepts text, so binary injection is not a risk via loadstring.
+-- However, loadfile() can load binary chunks in all versions — validate files first.
+```
+
+*Source: Lua 5.2 Manual §8.2 — "Lua does not have bytecode verification anymore. So, all functions that load code (load and loadfile) are potentially insecure when loading untrusted binary data."*
+
+## Global Namespace Leakage
+
+Every variable written without `local` is an implicit global in Lua. This has severe consequences:
+
+1. **Cross-addon contamination**: Any addon can read or overwrite your globals
+2. **Information leak**: Internal data structures become publicly visible
+3. **Taint**: In WoW, global writes from a tainted call stack propagate taint to the written variable, which then propagates to anything that reads it
+4. **Silent bugs**: Typos in variable names silently create new globals instead of erroring
+
+### Prevention
+
+```lua
+-- Use luacheck with std = "none" to catch ALL undeclared globals
+-- See hooks.md for .luacheckrc configuration
+
+-- BAD: Leaks NPC data into the global namespace
+QuestieX_WotLKDB_npc = addonTable.npcData  -- Visible to every addon!
+
+-- GOOD: Share via Plugin API (private channel)
+local plugin = QuestiePluginAPI:RegisterPlugin("WotLKDB")
+plugin.data = addonTable  -- Only accessible through the registry
+```
+
+### Runtime Global Access Monitoring (Development)
+
+For debugging, use a `__newindex` hook on `_G` to detect unexpected global writes:
+
+```lua
+-- WARNING: Development only — remove before release
+if DEBUG_MODE then
+    setmetatable(_G, {
+        __newindex = function(t, k, v)
+            local info = debug.getinfo(2, "Sl")
+            print(string.format(
+                "WARNING: Global write: %s = %s at %s:%d",
+                tostring(k), tostring(v),
+                info.short_src, info.currentline
+            ))
+            rawset(t, k, v)
+        end
+    })
+end
+```
+
+## Input Validation at System Boundaries
+
+Validate all external input — user text, network data, saved variable files, addon communication — before processing:
+
+```lua
+-- GOOD: Validate before use, return nil + error for invalid input
+local function safeGetNPC(npcId)
+    if type(npcId) ~= "number" then
+        return nil, "npcId must be a number, got: " .. type(npcId)
+    end
+    if npcId <= 0 or npcId ~= math.floor(npcId) then
+        return nil, "npcId must be a positive integer, got: " .. tostring(npcId)
+    end
+    return QuestieDB:GetNPC(npcId)
+end
+
+-- GOOD: Validate deserialized data structure
+local function validateConfig(config)
+    if type(config) ~= "table" then return nil, "config must be a table" end
+    if type(config.version) ~= "string" then return nil, "config.version must be a string" end
+    if type(config.maxLevel) ~= "number" then return nil, "config.maxLevel must be a number" end
+    if config.maxLevel < 1 or config.maxLevel > 100 then
+        return nil, "config.maxLevel out of range: " .. config.maxLevel
+    end
+    return config
+end
+```
+
+### SavedVariables Validation
+
+Always validate SavedVariables on load — they can be manually edited by users or corrupted:
+
+```lua
+function Questie:LoadSavedVariables()
+    local sv = QuestieSV
+    if type(sv) ~= "table" then
+        -- Corrupted or missing — reset to defaults
+        QuestieSV = self:GetDefaults()
+        return
+    end
+
+    -- Validate schema version
+    if type(sv.version) ~= "number" or sv.version < MIN_SV_VERSION then
+        -- Schema too old — migrate or reset
+        QuestieSV = self:MigrateSV(sv)
+    end
+end
+```
+
+## File I/O (Standalone Lua)
+
+In sandboxed environments (WoW), `io` is not available. In standalone scripts:
+
+### Path Traversal Prevention
+
+```lua
+-- BAD: User-controlled path — allows directory traversal
+local f = io.open(userPath, "r")
+
+-- GOOD: Validate path is within allowed directory
+local function safeOpen(filename, mode)
+    -- Strip path traversal attempts
+    if filename:find("%.%.") or filename:find("[/\\]") then
+        return nil, "Invalid filename: path traversal detected"
+    end
+    local fullPath = SAFE_DIRECTORY .. "/" .. filename
+    return io.open(fullPath, mode)
+end
+```
+
+### Resource Management
+
+Always close file handles to prevent resource exhaustion:
+
+```lua
+-- BAD: Handle leak on error
+local f = io.open("data.txt", "r")
+local content = f:read("*a")  -- If this errors, f is never closed
+f:close()
+
+-- GOOD: Protected read with guaranteed close
+local function readFile(path)
+    local f, err = io.open(path, "r")
+    if not f then return nil, err end
+
+    local ok, content = pcall(f.read, f, "*a")
+    f:close()  -- Always close, even if read failed
+
+    if not ok then return nil, content end
+    return content
+end
+```
+
+## Shell Injection (`os.execute` / `io.popen`)
+
+NEVER pass user-controlled strings to shell commands:
+
+```lua
+-- CRITICAL RISK: Shell injection
+os.execute("grep " .. userInput .. " /var/log/app.log")
+-- An attacker sends: "; rm -rf / #" as userInput
+
+-- CRITICAL RISK: Same with io.popen
+local handle = io.popen("curl " .. userUrl)
+
+-- SAFE: Use validated, sanitized inputs or avoid shell entirely
+local function safeLookup(word)
+    -- Validate: alphanumeric only
+    if not word:match("^%w+$") then
+        return nil, "Invalid input: must be alphanumeric"
+    end
+    -- Now safe to use in a controlled command
+    return os.execute("grep -w " .. word .. " dictionary.txt")
+end
+```
+
+## Sandbox Design for Plugin Systems
+
+When building systems that run third-party plugin code, restrict the execution environment:
+
+### Environment Restriction
+
+```lua
+-- Create a restricted environment for plugin execution
+local function createSandbox()
+    return {
+        -- Safe builtins
+        print    = print,
+        pairs    = pairs,
+        ipairs   = ipairs,
+        next     = next,
+        type     = type,
+        tostring = tostring,
+        tonumber = tonumber,
+        select   = select,
+        unpack   = unpack,
+        error    = error,
+        pcall    = pcall,
+
+        -- Safe libraries (read-only subsets)
+        string = {
+            format = string.format,
+            find   = string.find,
+            sub    = string.sub,
+            len    = string.len,
+            lower  = string.lower,
+            upper  = string.upper,
+        },
+        table = {
+            insert = table.insert,
+            remove = table.remove,
+            sort   = table.sort,
+            concat = table.concat,
+        },
+        math = {
+            floor = math.floor,
+            ceil  = math.ceil,
+            min   = math.min,
+            max   = math.max,
+            abs   = math.abs,
+        },
+
+        -- EXPLICITLY EXCLUDED:
+        -- os           (shell access, file system)
+        -- io           (file system access)
+        -- debug        (CRITICAL: debug.getupvalue/setupvalue can read/modify
+        --              any upvalue in any function, bypassing sandbox entirely.
+        --              debug.getlocal/setlocal can read/modify locals on the
+        --              call stack. debug.setmetatable bypasses __metatable guards.
+        --              Never expose ANY debug library function to untrusted code.)
+        -- load         (arbitrary code execution)
+        -- loadstring   (arbitrary code execution)
+        -- loadfile     (arbitrary code execution)
+        -- dofile       (arbitrary code execution)
+        -- rawget       (bypass metamethod guards)
+        -- rawset       (bypass metamethod guards)
+        -- setmetatable (override protections)
+        -- getmetatable (inspect protected tables)
+    }
+end
+
+-- Execute plugin code in sandbox
+-- Version-gated: setfenv for 5.0/5.1, _ENV wrapper for 5.2+
+local function runInSandbox(code, sandbox)
+    if setfenv then
+        -- Lua 5.0 / 5.1: setfenv directly restricts the environment
+        local fn, err = loadstring(code)
+        if not fn then return nil, "Compile error: " .. err end
+        setfenv(fn, sandbox)
+        return pcall(fn)
+    else
+        -- Lua 5.2+: use load() with custom _ENV
+        -- The 4th arg to load() sets the environment
+        local fn, err = load(code, "=(sandbox)", "t", sandbox)
+        if not fn then return nil, "Compile error: " .. err end
+        return pcall(fn)
+    end
+end
+```
+
+**Why this matters**: `setfenv`/`getfenv` were removed in Lua 5.2. Code that relies on `setfenv` for sandboxing will silently fail or error on 5.2+. Always use the version-gated pattern above.
+
+### Resource Limiting
+
+For untrusted code, add execution timeout via `debug.sethook` (available in Lua 5.0+):
+
+```lua
+local function runWithTimeout(fn, maxInstructions)
+    maxInstructions = maxInstructions or 1000000
+    local count = 0
+    debug.sethook(function()
+        count = count + 1
+        if count > maxInstructions then
+            error("Execution limit exceeded: suspected infinite loop")
+        end
+    end, "", 1)  -- Hook every instruction
+
+    local ok, result = pcall(fn)
+    debug.sethook()  -- Remove hook
+    return ok, result
+end
+```
+
+**Version note**: `debug.sethook` is available in Lua 5.0+ and works identically across all versions. The `debug` library itself may be stripped in sandboxed environments (WoW does not expose the full `debug` library to addons).
+
+## Dependency Security
+
+- Audit third-party Lua libraries (LuaRocks packages) for known CVEs before vendoring
+- Pin dependency versions in rockspec or lockfile — never use floating `latest`
+- Prefer small, auditable libraries over large frameworks for security-sensitive code
+- Vendor critical dependencies (copy into project) rather than relying on external resolution
+- Review transitive dependencies: `luarocks show <package>` lists deps
+
+```bash
+# List all installed packages and versions
+luarocks list
+
+# Show package info including dependencies
+luarocks show lpeg
+
+# Install specific version (avoid floating latest)
+luarocks install luacheck 1.1.2
+```
+
+## Error Message Security
+
+Never expose internal details in user-facing error messages:
+
+```lua
+-- BAD: Leaks internal path and database schema
+error("Failed to load NPC " .. npcId .. " from "
+    .. dbPath .. ": column 'rawdata' is nil at index " .. idx)
+
+-- GOOD: Generic user message, detailed internal log
+Questie:Debug(Questie.DEBUG_CRITICAL,
+    format("[QuestieDB] GetNPC failed: npcId=%d, rawdata=nil, source=%s",
+        npcId, dbPath))
+return nil  -- Return nil to caller, no internal details
+```
+
+## WoW-Specific: Taint and Secure Code
+
+Taint is a security mechanism in the WoW client that prevents addon code from executing protected actions (opening bags during combat, using abilities, etc.). Understanding taint is CRITICAL for WoW addon development.
+
+### Taint Propagation Rules
+
+1. Any variable written from insecure (addon) code is **tainted**
+2. Any variable read from tainted state becomes **tainted**
+3. Taint propagates through function calls, table reads, and variable assignments
+4. Protected API calls from a tainted call stack trigger `ADDON_ACTION_BLOCKED`
+
+### Common Taint Sources and Fixes
+
+| Taint Source | Why It Taints | Fix |
+|-------------|---------------|-----|
+| `_G.MyVar = value` | Global write from addon code | Use `addonTable` instead |
+| `loadstring(code)()` | Compiled code is always tainted | Use function dispatch tables |
+| Overwriting Blizzard functions | Replaces secure with insecure | Use `hooksecurefunc()` (post-hook) |
+| Writing from `OnUpdate` | Frequent tainted writes | Move to `ADDON_LOADED` or `PLAYER_LOGIN` |
+| `rawset(_G, k, v)` | Bypasses metamethods but still taints | Avoid; use local module tables |
+| Reading a tainted global | Taint propagates to reader | Read during `ADDON_LOADED` or cache locally |
+
+### Safe WoW API Patterns
+
+```lua
+-- GOOD: Post-hook (does not replace the original, does not taint)
+hooksecurefunc("QuestLogFrame_Update", function()
+    -- Your code runs AFTER the original — cannot taint it
+end)
+
+-- BAD: Function replacement (replaces secure with insecure = taint)
+local original = QuestLogFrame_Update
+QuestLogFrame_Update = function(...)  -- Now tainted!
+    original(...)
+    myCustomLogic()
+end
+
+-- GOOD: Combat guard for protected actions
+local function safeAction()
+    if InCombatLockdown() then
+        -- Queue for after combat
+        return
+    end
+    -- Safe to call protected APIs
+end
+```
+
+## Security Response Protocol
+
+If a security issue is found in Lua code:
+1. **STOP** immediately — do not ship the code
+2. Use **security-reviewer** agent
+3. Fix CRITICAL issues before continuing
+4. If secrets were exposed, rotate them immediately
+5. Audit the entire codebase for similar patterns
+6. Add `luacheck` rules to prevent recurrence
+7. Add regression tests for the specific vulnerability
+
+## References
+
+See skill: `security-review` for general security checklists applicable across all languages.