18 KiB
18 KiB
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 --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:<mediatype>;base64,... or data:<mediatype>,...)
+ // 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<string, unknown>} */ (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<void>} 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<void>} 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<void>} 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
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
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.