feat(security): warn once when enableSecureText is disabled + document XSS trade-off#474
Closed
gnbm wants to merge 8 commits into
Closed
feat(security): warn once when enableSecureText is disabled + document XSS trade-off#474gnbm wants to merge 8 commits into
gnbm wants to merge 8 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.
…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).
…ent (S4/P3/P4) P4: route fire-and-forget setTimeout calls through setManagedTimeout, whose ids are tracked and cleared in destroy(). Prevents callbacks (e.g. renderOptions, focus) from running against a destroyed instance. P3: document the intentional forced reflow in openDropbox so it is not removed as a no-op. S4: document that the search-highlight regex relies on enableSecureText for escaping and is not an additional injection vector (input is already regex-escaped). Adds cypress spec timer-cleanup.cy.ts.
…ed + document XSS trade-off (S1) enableSecureText is OFF by default and option label/value are rendered as raw HTML. Rather than change the default (disruptive, and escaping is costly on large 10k-100k+ datasets), log a single per-page console.warn when options are rendered while enableSecureText is disabled, so the XSS trade-off is discoverable. The check is O(1) and never scans option content. Also documents the trade-off loudly in docs/properties.md (top-of-page callout + expanded enableSecureText row). Adds cypress spec secure-text-warning.cy.ts.
Remove an unnecessary ts-expect-error in the timer cleanup test. In Utils.throttle, add JSDoc typings for timeout and lastArgs, initialize lastArgs as an array, annotate the throttled function args, and replace callback.apply(this, lastArgs) with callback(...lastArgs). These changes improve type-safety and simplify the callback invocation.
Add JSDoc type annotations for timeout and throttle args, initialize lastArgs as an array, and replace callback.apply(this, lastArgs) with callback(...lastArgs) to use spread invocation. Updated corresponding minified and docs assets (dist/virtual-select.js, dist/virtual-select.min.js, docs/assets/virtual-select.js, docs/assets/virtual-select.min.js). These changes improve type clarity and simplify argument handling in Utils.throttle.
Collaborator
Author
|
Closing in favour of #480. Almost everything here is already 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
enableSecureTextdefaults tofalse, so optionlabel/valueare written to the DOM as raw HTML. Consumers rendering untrusted data without opting in are exposed to XSS, and there's no signal that this is happening.Changing the default to
truewould be disruptive (breaks intentional-HTML consumers) and would impose per-option escaping cost on large datasets (10k–100k+ records) where it's often unnecessary. So per the agreed approach, this PR keeps the default off but makes the trade-off impossible to miss.Change
enableSecureTextis disabled, log a single (per page)console.warnpointing to the property docs. The check is O(1) — it never scans option content, so there's no cost on large datasets. Tracked by a staticsecureTextWarningShownflag.properties.mdplus an expandedenableSecureTextrow explaining the XSS risk, when to enable it, and the large-dataset performance rationale for the default.No behavioural/default change beyond the warning.
Verification
New spec
cypress/e2e/secure-text-warning.cy.ts:enableSecureTextoff;enableSecureText: true.Other Validations:
npm run validatefor static code validation:npm run build: succeeds;dist/docsrebuiltnpm run test):