# Blind Hunter — Story 4.2 Code Review You are a **Blind Hunter** adversary reviewer. You receive ONLY the diff below — no project context, no spec, no access to the codebase. Your job is to find bugs, vulnerabilities, logic errors, and questionable patterns in the code changes shown. ## Instructions - Review the diff strictly as presented. - Look for: logic errors, security issues, race conditions, type confusion, unhandled edge cases, performance problems, memory leaks, antipatterns, API misuse, DOM/XSS vulnerabilities. - Do NOT make assumptions about the codebase beyond what the diff reveals. - Report findings as a Markdown list. Each finding: **one-line title**, category (bug/security/antipattern/edge-case), location (file + line), and evidence from the diff. - If you find nothing, say "No issues found." ## Diff === MODIFIED FILES (git diff HEAD) === ```diff diff --git a/src/contracts/privacy-settings.js b/src/contracts/privacy-settings.js index b838445..f618241 100644 --- a/src/contracts/privacy-settings.js +++ b/src/contracts/privacy-settings.js @@ -5,15 +5,34 @@ * their on-screen presence. Settings are stored as user flags on the user document. * * Storage key: game.user.setFlag('video-view-manager', key, value) - * Shape: { reactionCamEnabled: boolean, hpReactiveCamStylingEnabled: boolean } + * Shape: { reactionCamEnabled: boolean, hpReactiveCamStylingEnabled: boolean, customPortraitFallback: string | null } * * @module contracts/privacy-settings */ +/** + * Maximum portrait file size in bytes (5MB). + * Note: FoundryVTT user flags typically have a ~50KB limit, so images should be optimized. + * @type {number} + */ +export const MAX_PORTRAIT_SIZE = 5 * 1024 * 1024; + +/** + * Supported portrait image MIME types. + * @type {string[]} + */ +export const VALID_PORTRAIT_FORMATS = Object.freeze([ + "image/png", + "image/jpeg", + "image/webp", + "image/gif", +]); + /** * @typedef {Object} PrivacySettings * @property {boolean} reactionCamEnabled - Whether Reaction Cam automation is enabled for this user. * @property {boolean} hpReactiveCamStylingEnabled - Whether HP-Reactive Cam Styling is enabled for this user. + * @property {string|null} customPortraitFallback - DataURL string for custom portrait fallback image, or null if not set. */ export const PRIVACY_SETTINGS_VERSION = 1; @@ -25,6 +44,7 @@ export const PRIVACY_SETTINGS_VERSION = 1; export const PRIVACY_SETTINGS_DEFAULT = { reactionCamEnabled: false, hpReactiveCamStylingEnabled: false, + customPortraitFallback: null, }; /** @@ -34,6 +54,7 @@ export const PRIVACY_SETTINGS_DEFAULT = { export const PRIVACY_SETTING_KEYS = Object.freeze([ "reactionCamEnabled", "hpReactiveCamStylingEnabled", + "customPortraitFallback", ]); /** @@ -45,6 +66,49 @@ export const FEATURE_NAME_MAP = Object.freeze({ hpReactiveCamStyling: "hpReactiveCamStylingEnabled", }); +/** + * Validates a DataURL for portrait images. + * Accepts DataURLs with supported MIME types or null/undefined/empty string. + * @param {unknown} dataURL - The DataURL string to validate. + * @returns {string|null|undefined} The validated DataURL (or null/undefined if valid). + * @throws {TypeError} If the DataURL format is invalid or uses unsupported MIME type. + */ +export function validatePortraitDataURL(dataURL) { + // Accept null, undefined, or empty string as valid (no custom portrait) + if (dataURL === null || dataURL === undefined) { + return dataURL; + } + if (typeof dataURL !== "string") { + throw new TypeError(`Invalid DataURL: expected string, got ${typeof dataURL}`); + } + if (dataURL === "") { + return dataURL; + } + + // Validate DataURL format: must start with "data:" + if (!dataURL.startsWith("data:")) { + throw new TypeError("Invalid DataURL format: must start with 'data:'"); + } + + // Extract MIME type from DataURL (format: data:;base64,... or data:,...) + // Match any MIME type after data: (captures the part before ; or ,) + const mimeMatch = dataURL.match(/^data:(image\/[a-zA-Z0-9+\-.]+|video\/[a-zA-Z0-9+\-.]+)/); + if (!mimeMatch) { + throw new TypeError("Invalid DataURL format: missing or invalid MIME type"); + } + + const mimeType = mimeMatch[1].toLowerCase(); + + // Validate against supported formats + if (!VALID_PORTRAIT_FORMATS.includes(mimeType)) { + throw new TypeError( + `Unsupported portrait format: ${mimeType}. Supported: ${VALID_PORTRAIT_FORMATS.join(", ")}` + ); + } + + return dataURL; +} + /** * Creates a new PrivacySettings object with defaults. * Only known keys from PRIVACY_SETTINGS_DEFAULT are included; extra properties are ignored. @@ -63,6 +127,7 @@ export function createPrivacySettings(overrides = {}) { /** * Validates a PrivacySettings DTO. Throws TypeError on any violation. + * Backward compatible: accepts settings without customPortraitFallback key. * @param {unknown} data - Value to validate. * @returns {PrivacySettings} The validated settings. * @throws {TypeError} If data fails validation. @@ -75,23 +140,50 @@ export function isValidPrivacySettings(data) { throw new TypeError("PrivacySettings: must be an object"); } const obj = /** @type {Record} */ (data); - const { reactionCamEnabled, hpReactiveCamStylingEnabled, ...rest } = obj; - if (Object.keys(rest).length > 0) { + + // Check for unknown keys (keys not in PRIVACY_SETTING_KEYS) + const knownKeys = new Set(PRIVACY_SETTING_KEYS); + const unknownKeys = Object.keys(obj).filter((k) => !knownKeys.has(k)); + if (unknownKeys.length > 0) { throw new TypeError( - `PrivacySettings: unknown keys: ${Object.keys(rest).join(", ")}` + `PrivacySettings: unknown keys: ${unknownKeys.join(", ")}` ); } - if (typeof reactionCamEnabled !== "boolean") { - throw new TypeError( - `PrivacySettings: reactionCamEnabled must be a boolean, got ${typeof reactionCamEnabled}` - ); + + // Validate each known key if present + if ("reactionCamEnabled" in obj) { + if (typeof obj.reactionCamEnabled !== "boolean") { + throw new TypeError( + `PrivacySettings: reactionCamEnabled must be a boolean, got ${typeof obj.reactionCamEnabled}` + ); + } } - if (typeof hpReactiveCamStylingEnabled !== "boolean") { - throw new TypeError( - `PrivacySettings: hpReactiveCamStylingEnabled must be a boolean, got ${typeof hpReactiveCamStylingEnabled}` - ); + if ("hpReactiveCamStylingEnabled" in obj) { + if (typeof obj.hpReactiveCamStylingEnabled !== "boolean") { + throw new TypeError( + `PrivacySettings: hpReactiveCamStylingEnabled must be a boolean, got ${typeof obj.hpReactiveCamStylingEnabled}` + ); + } } - return /** @type {PrivacySettings} */ (data); + if ("customPortraitFallback" in obj) { + if (obj.customPortraitFallback !== null && typeof obj.customPortraitFallback !== "string") { + throw new TypeError( + `PrivacySettings: customPortraitFallback must be a string or null, got ${typeof obj.customPortraitFallback}` + ); + } + // If it's a string, validate it's a valid DataURL + if (typeof obj.customPortraitFallback === "string" && obj.customPortraitFallback !== "") { + try { + validatePortraitDataURL(obj.customPortraitFallback); + } catch (e) { + throw new TypeError( + `PrivacySettings: customPortraitFallback ${e.message}` + ); + } + } + } + + return /** @type {PrivacySettings} */ (obj); } /** diff --git a/src/core/PlayerPrivacyManager.js b/src/core/PlayerPrivacyManager.js index 117afa2..9347edd 100644 --- a/src/core/PlayerPrivacyManager.js +++ b/src/core/PlayerPrivacyManager.js @@ -15,9 +15,11 @@ import { PRIVACY_SETTINGS_DEFAULT, PRIVACY_SETTING_KEYS, FEATURE_NAME_MAP, + MAX_PORTRAIT_SIZE, validateSettingKey, validateSettingValue, validateFeatureName, + validatePortraitDataURL, } from "../contracts/privacy-settings.js"; /** @@ -118,12 +120,19 @@ export class PlayerPrivacyManager { * Emits change event to subscribers after successful update. * * @param {string} userId - The user ID to update settings for. - * @param {string} key - The setting key (must be in PRIVACY_SETTING_KEYS). + * @param {string} key - The setting key (must be in PRIVACY_SETTING_KEYS and not customPortraitFallback). * @param {boolean} value - The new setting value. * @returns {Promise} Resolves when the setting is persisted. * @throws {TypeError} If key is invalid, value is not boolean, or user doesn't exist. */ async setSetting(userId, key, value) { + // Reject customPortraitFallback - use dedicated method instead + if (key === "customPortraitFallback") { + throw new TypeError( + "PlayerPrivacyManager: customPortraitFallback must use setPortraitFallback() method" + ); + } + // Validate key validateSettingKey(key); @@ -245,6 +254,149 @@ export class PlayerPrivacyManager { } } + /** + * Sets a custom portrait fallback DataURL for a user. + * + * Validates the DataURL format and MIME type before persistence. + * Emits change event with type 'portrait' to subscribers after successful update. + * + * @param {string} userId - The user ID to set portrait for. + * @param {string} dataURL - The DataURL string for the portrait image. + * @returns {Promise} Resolves when the setting is persisted. + * @throws {TypeError} If dataURL is invalid, user doesn't exist, or user doesn't support setFlag. + */ + async setPortraitFallback(userId, dataURL) { + // Validate DataURL format + validatePortraitDataURL(dataURL); + + // Get user + const user = this._adapter.users.get(userId); + if (!user) { + throw new TypeError( + `PlayerPrivacyManager: User '${userId}' not found` + ); + } + + // Validate user has setFlag method + if (typeof user.setFlag !== "function") { + throw new TypeError( + `PlayerPrivacyManager: User '${userId}' does not support setFlag` + ); + } + + // Get previous value for change event + const previousValue = this.getPortraitFallback(userId); + + // Persist the setting via user flag + await user.setFlag("video-view-manager", "customPortraitFallback", dataURL); + + // Notify subscribers with special portrait type + this._notifyPortraitChange(userId, dataURL, previousValue); + } + + /** + * Retrieves the custom portrait fallback DataURL for a user. + * + * @param {string} userId - The user ID to retrieve portrait for. + * @returns {string|null} The DataURL string, or null if not set. + */ + getPortraitFallback(userId) { + const user = this._adapter.users.get(userId); + + // Return null if user doesn't exist or has no getFlag + if (!user || typeof user.getFlag !== "function") { + return null; + } + + const dataURL = user.getFlag("video-view-manager", "customPortraitFallback"); + + // Validate the stored DataURL (defensive programming) + if (dataURL !== null && dataURL !== undefined) { + try { + // If it's a non-empty string, validate it + if (typeof dataURL === "string" && dataURL !== "") { + validatePortraitDataURL(dataURL); + } + return dataURL ?? null; + } catch (e) { + // Invalid stored DataURL - treat as null + console.warn( + `[ScryingPool] PlayerPrivacyManager: Invalid stored portrait DataURL for user '${userId}': ${e.message}` + ); + return null; + } + } + + return null; + } + + /** + * Convenience method to get portrait fallback as DataURL directly. + * Same as getPortraitFallback but with explicit null return type. + * + * @param {string} userId - The user ID to retrieve portrait for. + * @returns {string|null} The DataURL string, or null if not set. + */ + getPortraitFallbackDataURL(userId) { + return this.getPortraitFallback(userId); + } + + /** + * Removes the custom portrait fallback for a user. + * + * Emits change event with type 'portrait' to subscribers after successful removal. + * + * @param {string} userId - The user ID to remove portrait for. + * @returns {Promise} Resolves when the setting is removed. + * @throws {TypeError} If user doesn't exist or user doesn't support unsetFlag. + */ + async removePortraitFallback(userId) { + const user = this._adapter.users.get(userId); + if (!user) { + throw new TypeError( + `PlayerPrivacyManager: User '${userId}' not found` + ); + } + + // Validate user has unsetFlag method + if (typeof user.unsetFlag !== "function") { + throw new TypeError( + `PlayerPrivacyManager: User '${userId}' does not support unsetFlag` + ); + } + + // Get previous value for change event + const previousValue = this.getPortraitFallback(userId); + + // Remove the setting via user flag + await user.unsetFlag("video-view-manager", "customPortraitFallback"); + + // Notify subscribers with special portrait type + this._notifyPortraitChange(userId, null, previousValue); + } + + /** + * Notifies all subscribers of a portrait change. + * + * @private + * @param {string} userId - The user ID whose portrait changed. + * @param {string|null} newValue - The new portrait DataURL (or null if removed). + * @param {string|null} previousValue - The previous portrait DataURL (or null). + */ + _notifyPortraitChange(userId, newValue, previousValue) { + for (const callback of this._subscribers) { + try { + callback(userId, "customPortraitFallback", newValue, previousValue); + } catch (err) { + // Swallow subscriber errors to prevent one bad subscriber from breaking others + console.error( + `[ScryingPool] PlayerPrivacyManager portrait subscriber error:`, + err + ); + } + } + } + /** * Cleans up internal state. * Safe to call multiple times. ``` === NEW FILES (untracked) === **File: src/core/PortraitFallbackHandler.js** ```js export class PortraitFallbackHandler { constructor(adapter, playerPrivacyManager) { if (!adapter || typeof adapter !== "object") { throw new TypeError("PortraitFallbackHandler: adapter argument is required and must be an object"); } if (!playerPrivacyManager || typeof playerPrivacyManager !== "object") { throw new TypeError("PortraitFallbackHandler: playerPrivacyManager argument is required and must be an object"); } this._adapter = adapter; this._playerPrivacyManager = playerPrivacyManager; this._subscribers = new Set(); } getFallbackImageURL(userId) { const user = this._adapter.users.get(userId); if (!user) return null; const custom = this._playerPrivacyManager.getPortraitFallbackDataURL(userId); if (custom) return custom; if (user.avatar) return user.avatar; return DEFAULT_PLACEHOLDER_URL; } getFallbackImageElement(userId) { const url = this.getFallbackImageURL(userId); const user = this._adapter.users.get(userId); const img = document.createElement("img"); img.src = url; img.className = "sp-portrait-fallback"; img.dataset.spRole = "portrait-fallback"; if (user && user.name) { img.alt = `${user.name}'s portrait`; } else { img.alt = "Participant portrait"; } img.style.width = "100%"; img.style.height = "100%"; img.style.objectFit = "cover"; return img; } static validatePortraitFile(file) { if (!(file && typeof file === "object" && file.type && file.size !== undefined)) { return { valid: false, error: "Invalid file object" }; } if (!VALID_PORTRAIT_FORMATS.includes(file.type.toLowerCase())) { return { valid: false, error: `Unsupported format: ${file.type}. Supported: ${VALID_PORTRAIT_FORMATS.join(", ")}` }; } if (file.size > MAX_PORTRAIT_SIZE) { return { valid: false, error: `File is too large. Maximum size: ${MAX_PORTRAIT_SIZE / (1024 * 1024)}MB` }; } return { valid: true }; } static fileToDataURL(file) { return new Promise((resolve, reject) => { const reader = new FileReader(); reader.onload = () => { if (typeof reader.result === "string") resolve(reader.result); else reject(new TypeError("FileReader produced non-string result")); }; reader.onerror = () => reject(new TypeError("FileReader error: failed to read file")); reader.onabort = () => reject(new TypeError("FileReader error: read aborted")); reader.readAsDataURL(file); }); } onPortraitChange(callback) { this._subscribers.add(callback); return () => { this._subscribers.delete(callback); }; } _notifyPortraitChange(userId, newValue, previousValue) { for (const callback of this._subscribers) { try { callback(userId, newValue, previousValue); } catch (err) { console.error("[ScryingPool] PortraitFallbackHandler subscriber error:", err); } } } teardown() { this._subscribers.clear(); } } ``` **File: tests/helpers/playerPrivacyManagerMock.js** ```js export function createPlayerPrivacyManagerMock(overrides = {}) { const defaults = { getSettings: vi.fn((userId) => ({ reactionCamEnabled: false, hpReactiveCamStylingEnabled: false, customPortraitFallback: null, })), getPortraitFallback: vi.fn((userId) => null), getPortraitFallbackDataURL: vi.fn((userId) => null), setPortraitFallback: vi.fn().mockResolvedValue(undefined), removePortraitFallback: vi.fn().mockResolvedValue(undefined), setSetting: vi.fn().mockResolvedValue(undefined), isOptedIn: vi.fn((userId, feature) => false), getAllSettings: vi.fn(() => new Map()), onChange: vi.fn(() => () => {}), teardown: vi.fn(), ...overrides, }; return defaults; } ``` Note: Test files (privacy-settings.test.js, PlayerPrivacyManager.test.js, PortraitFallbackHandler.test.js) exist but their content is omitted to avoid test code review confusion. Focus on production code and the mock helper.