Skip to content

harden: idempotent destroy(), sanitize wrapper classNames, partial-init safety#479

Merged
gnbm merged 1 commit into
masterfrom
harden/lifecycle-classnames-coverage
Jun 9, 2026
Merged

harden: idempotent destroy(), sanitize wrapper classNames, partial-init safety#479
gnbm merged 1 commit into
masterfrom
harden/lifecycle-classnames-coverage

Conversation

@gnbm

@gnbm gnbm commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

Issue number: resolves #


What is the current behavior?

The #468#478 series fixed the reported leaks/crash/XSS correctly, but a few hardening gaps remain:

  • destroy() is idempotent only emergently (every step happens to be safe to repeat); there is no explicit guard, so a future non-idempotent edit — or the observer auto-destroy racing a manual destroy() — could break.
  • Option classNames are run through Utils.sanitizeClassNames, but the component-level class options (additionalClasses, additionalToggleButtonClasses, additionalDropboxClasses, additionalDropboxContainerClasses) are interpolated into class="…" without sanitization.
  • In afterRenderWrapper(), addEvents() (which calls registerInstance and installs the shared observer + page-level listeners) runs before setEleProps() / initDropboxPopover() / setValueMethod(). If one of those throws, the instance is registered but the caller has no handle to destroy() it — leaking the global listeners/observer.
  • No test exercised double-destroy() or the shared observer's primary job (auto-destroying an instance whose host is removed from the DOM without calling destroy()).

What is the new behavior?

  • destroy(): explicit isDestroyed early-return guard (set once in the constructor) so double-destroy and observer-vs-manual re-entrancy stay safe regardless of future edits.
  • render() / renderDropbox(): the four additional*Classes options now pass through Utils.sanitizeClassNames, matching the option-classNames sink. Legitimate class names are unchanged (only ", <, > are stripped).
  • afterRenderWrapper(): post-addEvents() initialization is wrapped in try/catch that calls this.destroy() and rethrows, so a failure after registerInstance can no longer leak the shared observer / global listeners. The success path is behaviorally identical.
  • Tests: observer-listener-lifecycle.cy.ts gains (1) a double-destroy() idempotency test and (2) a test asserting the shared observer auto-destroys an instance when its host node is removed without destroy() (back-reference cleared, activeInstances empty, hasGlobalListeners === false, domObserver === null).

Does this introduce a breaking change?

  • Yes
  • No

Other information

  • npm run validate (tsc strict + eslint + stylelint) and npm run build pass; dist/, docs/assets/, and dist-archive/ bundles were regenerated to stay in sync with source.
  • Full Cypress e2e suite is green: the new lifecycle tests pass 7/7, and examples.cy.ts passes 215/215 on an isolated run (two transient timeouts seen only under a resource-contended full run are pre-existing focus/visibility flakiness on code paths untouched here).

…it safety

Defense-in-depth follow-ups on the #468-#478 memory-leak/perf/security series
(all validated as correct; these are non-blocking hardening):

- destroy(): explicit isDestroyed guard so double-destroy and observer-vs-manual
  re-entrancy stay safe against future edits (was idempotent only emergently).
- render()/renderDropbox(): run additionalClasses, additionalToggleButtonClasses,
  additionalDropboxClasses and additionalDropboxContainerClasses through
  Utils.sanitizeClassNames, matching the option-classNames sink (attribute-injection
  defense-in-depth; no change for legitimate class names).
- afterRenderWrapper(): wrap post-addEvents() init in try/catch -> self-destroy on
  failure so a throw after registerInstance can't leak the shared observer / global
  listeners.
- tests: add double-destroy idempotency and shared-observer auto-destroy (host removed
  without destroy()) coverage to observer-listener-lifecycle.cy.ts.

npm run validate + build pass; full Cypress suite green (the 2 transient examples.cy.ts
timeouts under load pass deterministically on isolated re-run, 215/215).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Hardens VirtualSelect lifecycle and rendering against double-destroy/re-entrancy, partial initialization failures, and class attribute injection, and adds e2e coverage for observer/listener lifecycle behavior.

Changes:

  • Make destroy() explicitly idempotent via an isDestroyed guard.
  • Sanitize the component-level additional*Classes options using Utils.sanitizeClassNames.
  • Wrap post-addEvents() initialization in afterRenderWrapper() with try/catch that destroy()s on failure and rethrows.
  • Extend Cypress lifecycle tests to cover double-destroy and shared-observer auto-destroy when a host is removed.

Reviewed changes

Copilot reviewed 3 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/virtual-select.js Adds isDestroyed guard, sanitizes additional class options, and ensures partial-init failures self-teardown.
cypress/e2e/observer-listener-lifecycle.cy.ts Adds e2e coverage for double-destroy idempotency and observer-driven auto-destroy on DOM removal.
dist/virtual-select.js Generated bundle updated to reflect source changes (should not be committed in PRs per repo guidance).
dist/virtual-select.min.js Generated minified bundle updated (should not be committed in PRs per repo guidance).
docs/assets/virtual-select.js Generated docs bundle updated (should not be committed in PRs per repo guidance).
docs/assets/virtual-select.min.js Generated minified docs bundle updated (should not be committed in PRs per repo guidance).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread docs/assets/virtual-select.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants