fix(security): sanitize option classNames against attribute injection#471
Closed
gnbm wants to merge 3 commits into
Closed
fix(security): sanitize option classNames against attribute injection#471gnbm wants to merge 3 commits into
gnbm wants to merge 3 commits into
Conversation
customData.group_name and customData.description were interpolated directly into the option aria-label attribute without escaping. A double quote in those fields could break out of the attribute and inject markup/handlers even when enableSecureText was enabled - an XSS bypass. Route both fields through secureText() (same path as label/value). secureText is a no-op when enableSecureText is disabled, so behaviour is unchanged for consumers that intentionally pass raw text. Adds cypress regression spec security-customdata-xss.cy.ts.
… (S3)
Option classNames were concatenated into the class attribute unescaped, so a
consumer-provided value containing a double quote could break out of the attribute
and inject markup/handlers.
Add Utils.sanitizeClassNames() which strips the only characters that can break out
of a double-quoted attribute (" < >). Valid CSS class tokens never contain these,
so legitimate class names are unaffected (covered by a no-regression test).
Adds cypress regression spec security-classnames-xss.cy.ts.
…472) * perf: throttle global resize handler and guard against instance-less wrappers (P1) The window 'resize' listener ran onResizeMethod on every tick, which queried all .vscomp-ele-wrapper elements and recomputed each instance's options-container height - O(instances) layout work per resize event during a drag. It also assumed every wrapper's parent had a .virtualSelect and threw otherwise. - Add Utils.throttle and wrap the resize listener (~100ms) so the per-instance work runs at most ~10x/sec. - Guard onResizeMethod against wrappers whose instance is missing (teardown races). Closed instances are still updated (opening does not recompute height, so skipping them could leave a stale height on reopen) - throttling delivers the win without that regression risk. Adds cypress spec perf-resize-throttle.cy.ts (throttling, no-regression, guard). * Update utils.js Fix lint issues * Add typings to throttle and use spread Add JSDoc/type annotations to Utils.throttle (timeout and lastArgs) and annotate the throttled function parameter. Replace callback.apply(this, lastArgs) with callback(...lastArgs) for simpler invocation and improved type-safety. Updated the built/minified distribution and docs asset files accordingly. * Update virtual-select-1.2.0.min.js * fix(utils): preserve caller context in throttle (Copilot review) Invoke the throttled callback via callback.apply(lastThis, lastArgs) on both leading and trailing edges so instance methods used as throttled event handlers run with the expected 'this'. Add @this annotation to satisfy tsc. Regenerate dist/docs bundles. * fix: prevent Virtual Select memory leaks (document click listener + cleanup gaps) (#468) * fix: remove document click listener correctly to prevent memory leak onDocumentClick is added to `document` in the capture phase (addEvent(document, 'click', 'onDocumentClick', true)), but removeEvent called removeEventListener without the capture flag. removeEventListener only unregisters a listener when its capture flag matches the one used at registration, so the capture-phase document listener was never removed. As a result, every render/destroy cycle (e.g. repeatedly opening and closing the dropdown) left a new onDocumentClick listener bound to `document`. Each leaked listener retains the VirtualSelect instance and its detached dropbox DOM subtree, so event-listener and DOM-node counts grow without bound and are never reclaimed by GC. Fix: thread the capture flag through DomUtils.removeEvent and the VirtualSelect.removeEvent wrapper, and pass capture: true when removing the document click listener so add and remove match. * fix: ensure cleanup runs in inline mode and clear retained references on destroy Two additional memory-leak hardening fixes: 1. Install the self-destroy MutationObserver in every mode, not only when hasDropboxWrapper is true. Previously, inline-mode dropdowns had no observer, so removing the host element from the DOM without calling destroy() left all addEvents() listeners attached - including the capture-phase document click listener that retains the instance and its detached DOM. removeMutationObserver now disconnects based on the observer's existence. 2. On destroy(), clear the $dropboxWrapper.virtualSelect back-reference (set in setEleProps) before detaching the wrapper, and reset this.events. This breaks the instance <-> detached-DOM reference cycle so the instance and its DOM can be reclaimed. * fix: limit mutation observer to popup mode to avoid per-instance perf penalty Reverts the all-modes MutationObserver install. In the default dropboxWrapper:'self' mode every instance was registering a body-wide subtree observer, adding O(instances x mutations) overhead on pages with many selects. The reported document-click listener leak is fixed by the capture-flag correction in removeEvents(); the observer stays scoped to popup mode as before. * build: rebuild minified bundles with S2/S3/P1 fixes The branch committed the unminified dist/docs sources and the dist-archive copy with the security (S2 customData escaping, S3 classNames sanitization) and perf (P1 throttled resize) fixes, but the primary distributed bundles (dist/virtual-select.min.js and docs/assets/virtual-select.min.js) were left stale - consumers loading them got none of the fixes. Regenerated via 'npm run build' so all three min bundles share an identical body and match the source. * refactor: keep stable reference to throttled resize handler (Copilot review) The throttled wrapper passed inline to addEventListener was anonymous, so the resize listener could no longer be detached via removeEventListener (the prior VirtualSelect.onResizeMethod reference was removable). Store the throttled function on VirtualSelect.onResizeThrottled and register that reference instead, restoring removability for future cleanup. Declared as a typed static field so it passes tsc (checkJs + strict). * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Revert "Potential fix for pull request finding" This reverts commit 98c9649. * refactor: drop unnecessary static class field for onResizeThrottled (Copilot review) The static class-field declaration provided no type-safety benefit (the file is // @ts-nocheck, so tsc never checks it) and used ES2022 class-field syntax not present anywhere else in the codebase. The property is assigned at module init time via VirtualSelect.onResizeThrottled = ..., which is sufficient to expose a stable, removable reference. Runtime behaviour is unchanged. * fix(utils): release throttle references after invocation (Copilot review) throttle retained lastArgs/lastThis (e.g. a DOM Event passed to a resize handler) until the next call or indefinitely if never called again. Clear them after both the leading and trailing invocations so large arguments aren't held, matching the canonical underscore/lodash throttle. Behaviour is unchanged: both paths reassign lastArgs/lastThis at the top of every call before use. --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
gnbm
changed the base branch from
security/escape-customdata-attributes
to
master
Collaborator
Author
|
Closing as already resolved on |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
classNamesis concatenated into theclassattribute without sanitization:A
"in a consumer-suppliedclassNamesbreaks out of the attribute and can inject markup/event handlers.Fix
Utils.sanitizeClassNames()which strips the only characters that can break out of a double-quoted attribute —",<,>. Valid CSS class tokens never contain these characters, so legitimate class names are left completely untouched; only breakout attempts are neutralised. Applied whereclassNamesis added tooptionClasses.Verification
New spec
cypress/e2e/security-classnames-xss.cy.ts:classNamesvalue injects nothing and never executes (option still renders as a normal.vscomp-option);my-custom-class another-class) are preserved exactly.Other Validations:
npm run validatefor static code validation:npm run build: succeeds;dist/docsrebuiltnpm run test):