uberwald/scrying-pool
0
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 5 Wiki Activity

Fix code review findings for Story 3.3: Preset Import & Export

Browse Source 
Security & Quality Improvements:
- Fix XSS vulnerabilities in PresetImportDialog, PresetExportDialog, and templates
- Add resource leak protection in downloadExportFile() with try/finally
- Fix encapsulation violation by using public API instead of _presetsCache
- Add rollback mechanism for partial failures in replace mode
- Add preset name validation (length, characters, empty check)
- Add duplicate name detection within import files
- Add file size validation (5MB limit) and type validation
- Fix event listener leaks with proper cleanup in _onRender/_onClose
- Add constructor parameter validation for all dialogs

Acceptance Criteria Compliance:
- Fix AC-2: Export filename now uses world name (via parent.name)
- Fix AC-6: Error message matches spec exactly ('Import failed: invalid JSON format')
- Fix AC-8: Merge/Replace messages match spec format

Code Quality:
- Add shared HTML escaping utilities (src/utils/html.js)
- Consolidate duplicate localization strings (removed 28 duplicates from SCRYING_POOL)
- Use SCENE_PRESET_VERSION constant instead of hardcoded 1
- Handle null options in importPresets()
- Graceful handling of skipValidation with invalid data

Test Results: 679 passed, 3 failed (pre-existing in DirectorsBoard)

Generated by Mistral Vibe.
Co-Authored-By: Mistral Vibe <vibe@mistral.ai>
This commit is contained in:
LeRatierBretonnier
2026-05-23 18:57:32 +02:00
parent a1e8886fce
commit ea2c2ea093
7 changed files with 354 additions and 131 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/ui/gm/PresetExportDialog.js
+44 -9
View File
@@ -8,6 +8,7 @@
*/
import { PresetImportExportManager } from '../../core/PresetImportExportManager.js';
import { escapeHtml } from '../../utils/html.js';
// Conditional base class — test environment lacks foundry globals.
// At module load time in tests, foundry is undefined → fallback class is used.
@@ -48,6 +49,9 @@ export class PresetExportDialog extends _AppBase {
constructor(options = {}) {
super(options);
if (!options || typeof options !== 'object') {
throw new TypeError('PresetExportDialog: options argument is required and must be an object');
}
if (!options.adapter || typeof options.adapter !== 'object') {
throw new TypeError('PresetExportDialog: adapter option is required and must be an object');
}
@@ -58,6 +62,10 @@ export class PresetExportDialog extends _AppBase {
this._adapter = options.adapter;
this._scenePresetManager = options.scenePresetManager;
this._exportManager = new PresetImportExportManager(this._adapter, this._scenePresetManager);
// Event listener tracking for cleanup
/** @type {Array<{element: Element, type: string, listener: EventListener}>} */
this._eventListeners = [];
}
static DEFAULT_OPTIONS = {
@@ -98,11 +106,22 @@ export class PresetExportDialog extends _AppBase {
const root = this.element;
if (!root) return;
// Clean up previous event listeners to prevent memory leaks
for (const { element, type, listener } of this._eventListeners) {
element.removeEventListener(type, listener);
}
this._eventListeners = [];
// Export button click handler
root.querySelector('.sp-export-btn')?.addEventListener('click', async (event) => {
event.preventDefault();
await this._onExport();
});
const exportBtn = root.querySelector('.sp-export-btn');
if (exportBtn) {
const handler = async (event) => {
event.preventDefault();
await this._onExport();
};
exportBtn.addEventListener('click', handler);
this._eventListeners.push({ element: exportBtn, type: 'click', listener: handler });
}
}
/**
@@ -111,6 +130,17 @@ export class PresetExportDialog extends _AppBase {
*/
async _onClose(options) {
await super._onClose?.(options);
// Clean up event listeners
for (const { element, type, listener } of this._eventListeners) {
try {
element.removeEventListener(type, listener);
} catch {
// Ignore errors during cleanup
}
}
this._eventListeners = [];
// Clean up any references
this._exportManager = null;
this._scenePresetManager = null;
@@ -130,15 +160,20 @@ export class PresetExportDialog extends _AppBase {
const originalLabel = btn.innerHTML;
try {
// Disable button and show loading state
// Disable button and show loading state (using DOM methods for safety)
btn.disabled = true;
btn.innerHTML = '<i class="fas fa-spinner fa-spin"></i> Exporting...';
btn.textContent = '';
const spinner = document.createElement('i');
spinner.className = 'fas fa-spinner fa-spin';
const text = document.createTextNode(' Exporting...');
btn.appendChild(spinner);
btn.appendChild(text);
// Export presets
const jsonString = await this._exportManager.exportAllPresets();
const currentScene = this._adapter.scenes.current?.();
const sceneName = currentScene?.name ?? 'world';
const filename = this._exportManager.generateExportFilename(sceneName);
const worldName = this._adapter.scenes.current?.()?.parent?.name ?? currentScene?.name ?? 'world';
const filename = this._exportManager.generateExportFilename(worldName);
// Trigger download
this._exportManager.downloadExportFile(jsonString, filename);
@@ -153,7 +188,7 @@ export class PresetExportDialog extends _AppBase {
} catch (err) {
const errorMsg = err instanceof Error ? err.message : String(err);
if (this._adapter.notifications) {
this._adapter.notifications.error('Failed to export presets: ' + errorMsg);
this._adapter.notifications.error('Failed to export presets: ' + escapeHtml(errorMsg));
}
} finally {
if (btn) {
src/ui/gm/PresetImportDialog.js
+121 -32
View File
@@ -8,6 +8,12 @@
*/
import { PresetImportExportManager } from '../../core/PresetImportExportManager.js';
import { escapeHtml } from '../../utils/html.js';
// Maximum file size: 5MB
const MAX_FILE_SIZE = 5 * 1024 * 1024;
// Allowed MIME types for JSON files
const ALLOWED_MIME_TYPES = new Set(['application/json', 'text/plain', '']);
// Conditional base class — test environment lacks foundry globals.
// At module load time in tests, foundry is undefined → fallback class is used.
@@ -48,6 +54,9 @@ export class PresetImportDialog extends _AppBase {
constructor(options = {}) {
super(options);
if (!options || typeof options !== 'object') {
throw new TypeError('PresetImportDialog: options argument is required and must be an object');
}
if (!options.adapter || typeof options.adapter !== 'object') {
throw new TypeError('PresetImportDialog: adapter option is required and must be an object');
}
@@ -68,6 +77,10 @@ export class PresetImportDialog extends _AppBase {
this._previewItems = [];
/** @type {boolean} */
this._requiresConfirmation = false;
// Event listener tracking for cleanup
/** @type {Array<{element: Element, type: string, listener: EventListener}>} */
this._eventListeners = [];
}
static DEFAULT_OPTIONS = {
@@ -111,41 +124,75 @@ export class PresetImportDialog extends _AppBase {
const root = this.element;
if (!root) return;
// Clean up previous event listeners to prevent memory leaks
for (const { element, type, listener } of this._eventListeners) {
element.removeEventListener(type, listener);
}
this._eventListeners = [];
// File input change handler
root.querySelector('.sp-file-input')?.addEventListener('change', (event) => {
this._onFileSelected(event);
});
const fileInput = root.querySelector('.sp-file-input');
if (fileInput) {
const handler = (event) => this._onFileSelected(event);
fileInput.addEventListener('change', handler);
this._eventListeners.push({ element: fileInput, type: 'change', listener: handler });
}
// Mode radio button handlers
root.querySelector('.sp-mode-merge')?.addEventListener('change', () => {
this._mode = 'merge';
this._requiresConfirmation = false;
this.render();
});
const mergeRadio = root.querySelector('.sp-mode-merge');
if (mergeRadio) {
const handler = () => {
this._mode = 'merge';
this._requiresConfirmation = false;
this.render();
};
mergeRadio.addEventListener('change', handler);
this._eventListeners.push({ element: mergeRadio, type: 'change', listener: handler });
}
root.querySelector('.sp-mode-replace')?.addEventListener('change', () => {
this._mode = 'replace';
this._requiresConfirmation = true;
this.render();
});
const replaceRadio = root.querySelector('.sp-mode-replace');
if (replaceRadio) {
const handler = () => {
this._mode = 'replace';
this._requiresConfirmation = true;
this.render();
};
replaceRadio.addEventListener('change', handler);
this._eventListeners.push({ element: replaceRadio, type: 'change', listener: handler });
}
// Import button click handler
root.querySelector('.sp-import-btn:not(.sp-confirm-btn)')?.addEventListener('click', async (event) => {
event.preventDefault();
await this._onImport();
});
const importBtn = root.querySelector('.sp-import-btn:not(.sp-confirm-btn)');
if (importBtn) {
const handler = async (event) => {
event.preventDefault();
await this._onImport();
};
importBtn.addEventListener('click', handler);
this._eventListeners.push({ element: importBtn, type: 'click', listener: handler });
}
// Confirm button handler (for replace mode)
root.querySelector('.sp-confirm-btn')?.addEventListener('click', async (event) => {
event.preventDefault();
await this._onConfirmImport();
});
const confirmBtn = root.querySelector('.sp-confirm-btn');
if (confirmBtn) {
const handler = async (event) => {
event.preventDefault();
await this._onConfirmImport();
};
confirmBtn.addEventListener('click', handler);
this._eventListeners.push({ element: confirmBtn, type: 'click', listener: handler });
}
// Cancel button handler
root.querySelector('.sp-cancel-btn')?.addEventListener('click', (event) => {
event.preventDefault();
this.close();
});
const cancelBtn = root.querySelector('.sp-cancel-btn');
if (cancelBtn) {
const handler = (event) => {
event.preventDefault();
this.close();
};
cancelBtn.addEventListener('click', handler);
this._eventListeners.push({ element: cancelBtn, type: 'click', listener: handler });
}
}
/**
@@ -154,6 +201,17 @@ export class PresetImportDialog extends _AppBase {
*/
async _onClose(options) {
await super._onClose?.(options);
// Clean up event listeners
for (const { element, type, listener } of this._eventListeners) {
try {
element.removeEventListener(type, listener);
} catch {
// Ignore errors during cleanup
}
}
this._eventListeners = [];
// Clean up any references
this._exportManager = null;
this._scenePresetManager = null;
@@ -178,7 +236,33 @@ export class PresetImportDialog extends _AppBase {
return;
}
this._selectedFile = input.files[0];
const file = input.files[0];
// Validate file type
if (!ALLOWED_MIME_TYPES.has(file.type)) {
// Check file extension as fallback
const validExtensions = ['.json'];
const hasValidExtension = validExtensions.some(ext => file.name.toLowerCase().endsWith(ext));
if (!hasValidExtension) {
if (this._adapter.notifications) {
this._adapter.notifications.error('Please select a JSON file (.json extension required)');
}
// Clear the input so user can select again
input.value = '';
return;
}
}
// Validate file size
if (file.size > MAX_FILE_SIZE) {
if (this._adapter.notifications) {
this._adapter.notifications.error(`File is too large (${Math.round(file.size / 1024 / 1024)}MB). Maximum size is ${Math.round(MAX_FILE_SIZE / 1024 / 1024)}MB.`);
}
input.value = '';
return;
}
this._selectedFile = file;
this._previewItems = [];
this._parseAndPreviewFile();
}
@@ -218,7 +302,7 @@ export class PresetImportDialog extends _AppBase {
}
} catch (err) {
valid = false;
error = err instanceof Error ? err.message : String(err);
error = escapeHtml(err instanceof Error ? err.message : String(err));
}
this._previewItems.push({ name, valid, error });
@@ -234,7 +318,7 @@ export class PresetImportDialog extends _AppBase {
this.render();
} catch (err) {
const errorMsg = err instanceof Error ? err.message : String(err);
this._previewItems = [{ name: this._selectedFile.name, valid: false, error: errorMsg }];
this._previewItems = [{ name: this._selectedFile.name, valid: false, error: escapeHtml(errorMsg) }];
this._requiresConfirmation = false;
this.render();
}
@@ -314,9 +398,14 @@ export class PresetImportDialog extends _AppBase {
const originalLabel = btn.innerHTML;
try {
// Disable button and show loading state
// Disable button and show loading state (using textContent for safety)
btn.disabled = true;
btn.innerHTML = '<i class="fas fa-spinner fa-spin"></i> Importing...';
btn.textContent = '';
const spinner = document.createElement('i');
spinner.className = 'fas fa-spinner fa-spin';
const text = document.createTextNode(' Importing...');
btn.appendChild(spinner);
btn.appendChild(text);
// Read and import file
const content = await this._readFileAsText(this._selectedFile);
@@ -329,7 +418,7 @@ export class PresetImportDialog extends _AppBase {
this.close();
} else {
// Show errors
const errorMessages = result.errors.join('\n');
const errorMessages = result.errors.map(e => escapeHtml(e)).join('\n');
if (this._adapter.notifications) {
this._adapter.notifications.error('Failed to import presets\n' + errorMessages);
}
@@ -337,7 +426,7 @@ export class PresetImportDialog extends _AppBase {
} catch (err) {
const errorMsg = err instanceof Error ? err.message : String(err);
if (this._adapter.notifications) {
this._adapter.notifications.error('Failed to import presets: ' + errorMsg);
this._adapter.notifications.error('Failed to import presets: ' + escapeHtml(errorMsg));
}
} finally {
btn.disabled = false;