From f928f848e57e7f636b0801743cc078f661244543 Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Thu, 15 Jan 2026 04:56:22 +0000 Subject: [PATCH] feat(security): add input validation for configuration settings - Implement `validateNumber` and `validateColor` in `ConfigManager`. - Clamp numeric settings (zoom, size, borders) to safe ranges. - Validate color settings against hex regex. - Update `ConfigManager` unit tests to cover validation logic. This enhancement prevents invalid configuration states and potential stability issues caused by malformed or extreme setting values. --- .jules/sentinel.md | 5 +++ src/magnify-glass/ConfigManager.ts | 62 ++++++++++++++++++++----- tests/unit/ConfigManager.test.ts | 72 +++++++++++++++++++++++++++++- 3 files changed, 128 insertions(+), 11 deletions(-) diff --git a/.jules/sentinel.md b/.jules/sentinel.md index c26da33..7e0d529 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -4,3 +4,8 @@ **Vulnerability:** Found multiple instances of `innerHTML` being used with unsanitized user inputs (Node titles, widget values) in `UIManager.ts`. **Learning:** This project uses direct DOM manipulation without a framework, making XSS a primary risk. Developers were manually building HTML strings. **Prevention:** Introduced `escapeHtml` utility. Any new code using `innerHTML` MUST sanitize inputs. Prefer `textContent` where possible, or use the `escapeHtml` helper. + +## 2026-01-07 - Unvalidated Configuration Inputs +**Vulnerability:** `ConfigManager` loaded settings from user storage without validation, allowing invalid values (e.g., negative dimensions, non-hex colors) that could degrade stability or rendering. +**Learning:** Configuration data from local storage/user settings must be treated as untrusted input. +**Prevention:** Implemented strict input validation (clamping for numbers, regex for hex colors) in `ConfigManager.loadSettings`. diff --git a/src/magnify-glass/ConfigManager.ts b/src/magnify-glass/ConfigManager.ts index 794f128..ec143bd 100644 --- a/src/magnify-glass/ConfigManager.ts +++ b/src/magnify-glass/ConfigManager.ts @@ -4,7 +4,7 @@ * Manages configuration and settings for the magnifying glass. */ -import { getSettingValue } from '../shared/utils'; +import { getSettingValue, clamp } from '../shared/utils'; import { STORAGE_KEYS } from '../shared/constants'; import type { GlassPosition, GlassShape, TextureFilter } from '../shared/constants'; @@ -203,15 +203,25 @@ export class ConfigManager { * Load settings from ComfyUI settings system. */ loadSettings(): void { - this.zoomFactor = getSettingValue("🔍MagnifyGlass.ZoomFactor", this.zoomFactor * 100) / 100; - this.glassSize = getSettingValue("🔍MagnifyGlass.GlassSize", this.glassSize); - this.borderColor = getSettingValue("🔍MagnifyGlass.BorderColor", this.borderColor); - this.borderWidth = getSettingValue("🔍MagnifyGlass.BorderWidth", this.borderWidth); + const rawZoom = getSettingValue("🔍MagnifyGlass.ZoomFactor", this.zoomFactor * 100); + this.zoomFactor = this.validateNumber(rawZoom, 10, 5000, 300) / 100; + + const rawGlassSize = getSettingValue("🔍MagnifyGlass.GlassSize", this.glassSize); + this.glassSize = this.validateNumber(rawGlassSize, 50, 2000, 300); + + const rawBorderColor = getSettingValue("🔍MagnifyGlass.BorderColor", this.borderColor); + this.borderColor = this.validateColor(rawBorderColor, "#6b7280"); + + const rawBorderWidth = getSettingValue("🔍MagnifyGlass.BorderWidth", this.borderWidth); + this.borderWidth = this.validateNumber(rawBorderWidth, 0, 50, 1); + this.activationKey = getSettingValue("🔍MagnifyGlass.ActivationKey", this.activationKey); this.altRequired = getSettingValue("🔍MagnifyGlass.AltRequired", this.altRequired); this.followCursor = getSettingValue("🔍MagnifyGlass.FollowCursor", this.followCursor); - this.offsetStep = getSettingValue("🔍MagnifyGlass.OffsetStep", this.offsetStep); + const rawOffsetStep = getSettingValue("🔍MagnifyGlass.OffsetStep", this.offsetStep); + this.offsetStep = this.validateNumber(rawOffsetStep, 1, 100, 5); + this.glassPosition = getSettingValue("🔍MagnifyGlass.GlassPosition", this.glassPosition); this.resetKey = getSettingValue("🔍MagnifyGlass.ResetKey", this.resetKey); this.glassShape = getSettingValue("🔍MagnifyGlass.GlassShape", this.glassShape); @@ -227,18 +237,50 @@ export class ConfigManager { this.accessibilityEnabled = getSettingValue("🔍MagnifyGlass.AccessibilityEnabled", this.accessibilityEnabled); this.highContrastMode = getSettingValue("🔍MagnifyGlass.HighContrastMode", this.highContrastMode); this.textGlowEnabled = getSettingValue("🔍MagnifyGlass.TextGlowEnabled", this.textGlowEnabled); - this.textGlowColor = getSettingValue("🔍MagnifyGlass.TextGlowColor", this.textGlowColor); - this.textGlowIntensity = getSettingValue("🔍MagnifyGlass.TextGlowIntensity", this.textGlowIntensity); - this.fontScaleFactor = getSettingValue("🔍MagnifyGlass.FontScaleFactor", this.fontScaleFactor); + + const rawGlowColor = getSettingValue("🔍MagnifyGlass.TextGlowColor", this.textGlowColor); + this.textGlowColor = this.validateColor(rawGlowColor, "#ffff00"); + + const rawGlowIntensity = getSettingValue("🔍MagnifyGlass.TextGlowIntensity", this.textGlowIntensity); + this.textGlowIntensity = this.validateNumber(rawGlowIntensity, 1, 50, 5); + + const rawFontScale = getSettingValue("🔍MagnifyGlass.FontScaleFactor", this.fontScaleFactor); + this.fontScaleFactor = this.validateNumber(rawFontScale, 50, 500, 100); + this.boldTextEnabled = getSettingValue("🔍MagnifyGlass.BoldTextEnabled", this.boldTextEnabled); this.textOutlineEnabled = getSettingValue("🔍MagnifyGlass.TextOutlineEnabled", this.textOutlineEnabled); - this.textOutlineColor = getSettingValue("🔍MagnifyGlass.TextOutlineColor", this.textOutlineColor); + + const rawOutlineColor = getSettingValue("🔍MagnifyGlass.TextOutlineColor", this.textOutlineColor); + this.textOutlineColor = this.validateColor(rawOutlineColor, "#000000"); + this.nodeTitleEmphasis = getSettingValue("🔍MagnifyGlass.NodeTitleEmphasis", this.nodeTitleEmphasis); this.invertColors = getSettingValue("🔍MagnifyGlass.InvertColors", this.invertColors); this.grayscaleMode = getSettingValue("🔍MagnifyGlass.GrayscaleMode", this.grayscaleMode); this.reduceMotion = getSettingValue("🔍MagnifyGlass.ReduceMotion", this.reduceMotion); } + /** + * Validate numeric input with bounds checking. + */ + private validateNumber(value: unknown, min: number, max: number, fallback: number): number { + const num = Number(value); + if (isNaN(num)) return fallback; + return clamp(num, min, max); + } + + /** + * Validate color string (hex). + */ + private validateColor(color: unknown, fallback: string): string { + if (!color || typeof color !== 'string') return fallback; + // Basic hex validation: #RGB, #RRGGBB, #RRGGBBAA + // We permit 3, 4, 6, or 8 hex digits + if (/^#([0-9A-Fa-f]{3}|[0-9A-Fa-f]{4}|[0-9A-Fa-f]{6}|[0-9A-Fa-f]{8})$/.test(color)) { + return color; + } + return fallback; + } + /** * Load saved offsets from localStorage. */ diff --git a/tests/unit/ConfigManager.test.ts b/tests/unit/ConfigManager.test.ts index ff601fd..8a3f31e 100644 --- a/tests/unit/ConfigManager.test.ts +++ b/tests/unit/ConfigManager.test.ts @@ -6,6 +6,9 @@ import { describe, it, expect, beforeEach, vi } from 'vitest'; +// Mock settings store +const settingsStore: Record = {}; + // Mock localStorage const localStorageMock = (() => { let store: Record = {}; @@ -22,7 +25,7 @@ vi.mock('/scripts/app.js', () => ({ app: { ui: { settings: { - getSettingValue: (key: string) => undefined + getSettingValue: (key: string) => settingsStore[key] } } } @@ -41,6 +44,8 @@ describe('ConfigManager', () => { beforeEach(() => { localStorageMock.clear(); + // Clear settings store + for (const key in settingsStore) delete settingsStore[key]; configManager = new ConfigManager(); }); @@ -62,6 +67,71 @@ describe('ConfigManager', () => { }); }); + describe('loadSettings Validation', () => { + it('should load valid settings correctly', () => { + settingsStore['🔍MagnifyGlass.ZoomFactor'] = 500; + settingsStore['🔍MagnifyGlass.GlassSize'] = 400; + + configManager.loadSettings(); + + expect(configManager.zoomFactor).toBe(5); + expect(configManager.glassSize).toBe(400); + }); + + it('should clamp values exceeding maximum', () => { + settingsStore['🔍MagnifyGlass.ZoomFactor'] = 10000; // Way too high + settingsStore['🔍MagnifyGlass.GlassSize'] = 5000; // Way too big + settingsStore['🔍MagnifyGlass.BorderWidth'] = 100; // Too thick + settingsStore['🔍MagnifyGlass.OffsetStep'] = 200; // Too fast + + configManager.loadSettings(); + + expect(configManager.zoomFactor).toBe(50); // Clamped to 5000 (50x) + expect(configManager.glassSize).toBe(2000); // Clamped to 2000 + expect(configManager.borderWidth).toBe(50); // Clamped to 50 + expect(configManager.offsetStep).toBe(100); // Clamped to 100 + }); + + it('should clamp values below minimum', () => { + settingsStore['🔍MagnifyGlass.ZoomFactor'] = -100; + settingsStore['🔍MagnifyGlass.GlassSize'] = 10; + settingsStore['🔍MagnifyGlass.BorderWidth'] = -5; + settingsStore['🔍MagnifyGlass.OffsetStep'] = 0; + + configManager.loadSettings(); + + expect(configManager.zoomFactor).toBe(0.1); // Clamped to 10 (0.1x) + expect(configManager.glassSize).toBe(50); // Clamped to 50 + expect(configManager.borderWidth).toBe(0); // Clamped to 0 + expect(configManager.offsetStep).toBe(1); // Clamped to 1 + }); + + it('should validate hex colors', () => { + settingsStore['🔍MagnifyGlass.BorderColor'] = '#ff0000'; // Valid + configManager.loadSettings(); + expect(configManager.borderColor).toBe('#ff0000'); + + settingsStore['🔍MagnifyGlass.BorderColor'] = 'invalid-color'; // Invalid + configManager.loadSettings(); + expect(configManager.borderColor).toBe('#6b7280'); // Fallback to default (initialized in constructor) + + settingsStore['🔍MagnifyGlass.BorderColor'] = '#123'; // Valid short hex + configManager.loadSettings(); + expect(configManager.borderColor).toBe('#123'); + + settingsStore['🔍MagnifyGlass.BorderColor'] = '#12345678'; // Valid alpha hex + configManager.loadSettings(); + expect(configManager.borderColor).toBe('#12345678'); + }); + + it('should handle non-numeric inputs gracefully', () => { + settingsStore['🔍MagnifyGlass.GlassSize'] = "not a number"; + configManager.loadSettings(); + // Should fallback to default (300) or previous value + expect(configManager.glassSize).toBe(300); + }); + }); + describe('loadSavedOffsets', () => { it('should load offsets from localStorage', () => { localStorageMock.setItem('comfyui_magnify_offset_x', '50');